Internal: Configure tests on a per-test basis

[rellafella]

Tests can't be configured with prettier options on a per-test basis #43

🏗️ this is still under construction but I wanted to get the draft PR in to explain a bit.

The way this is working at the moment in the pull request is instead of writing to a jsfmt.snap.js file, it is using the expec.toMatchFileSnapshot() method. https://vitest.dev/guide/snapshot#file-snapshots
See the current status of the files to see how that looks.

I have also got this working in a way that is more consistent with how it was setup before, by having one snapshot that will contain all of the tests. See the below screenshot for how that would work.

I could go either way on this, neither really change the workflow and the snapshot can be updated by passing the -u flag.

[rellafella]

I am curious about the "Failing" tests, not quite sure if they should be in here but rather listed out as issues...

[zackad]

What do you think about putting snapshot files next to source? some thing like

jsfmt.spec.js
input.twig
input.snap.twig

Or we can use .snap as file extension, but we might lose syntax hightlighting on the editor. I think this will make it easier to compare the input with expected output.

[rellafella]

What do you think about putting snapshot files next to source?

This is very easy to configure, I actually started it like this as it resembles the prettier-pug approach, however having 2 files side-by-side where one is generated and the other is not, seemed like trouble. That being said, happy to move to that format if you would like.

I'll continue down this path for now, and once this is passing all tests i'll look at restructuring it.

Confirming though, you're happy with a snapshot file that is in a twig format instead of just exporting a string in a JS file?

[rellafella]

I would suggest checking this out and having a bit of a play, perhaps even writing a new test or two. If you want me to remove the __snapshots__ dir and inline those snapshot files I can do that too.

Do you have an opinion on whether every test should have an associated snapshot? I think it would be useful for some tests just confirming for a "no-change" so instead of comparing to the snapshot it would just be testing expect(actual).toBe(code). This would be particularly useful for testing multiple values for options.

[zackad]

Confirming though, you're happy with a snapshot file that is in a twig format instead of just exporting a string in a JS file?

yes, having snapshot file as twig file will make it easier compare via diff command or text editor.

[zackad]

"should handle twig comments"

[zackad]

I'm not familiar with jsconfig stuff. Is this vscode specific features or it being used by other tools. I'm courious.

[rellafella]

I believe it actually stems from TypeScript, but is tied closely to VScode since it supports some aspects of TypeScript within JS from an IDE perspective.
https://www.typescriptlang.org/docs/handbook/tsconfig-json.html.

[zackad]

Having extra whitespace after <!-- and berfore --> on multiline html comment. Is this the expected output? I never write multiline comment. The old snapshot seems to be that way.

[rellafella]

I also thought this was a little odd, as well as some of the other tests.
I have gone for the approach of matching the current snapshots. But an alternate way would be that we have some tests that are skipped, and set up the snapshot to be a more "expected" output, which would give some good milestones for a v1 release.

[zackad]

This is very easy to configure, I actually started it like this as it resembles the prettier-pug approach, however having 2 files side-by-side where one is generated and the other is not, seemed like trouble. That being said, happy to move to that format if you would like.

If you want me to remove the __snapshots__ dir and inline those snapshot files I can do that too.

I don't have strong oponion on this. Either way is fine.

Do you have an opinion on whether every test should have an associated snapshot? I think it would be useful for some tests just confirming for a "no-change" so instead of comparing to the snapshot it would just be testing expect(actual).toBe(code). This would be particularly useful for testing multiple values for options.

If the test doesn't change anything, I think it's fine not to include snapshot file.

[zackad]

Do you have anything to add? I think this is ready to be merged. We can deal other "inconsistency" on other PR. Before that, please rebase so I can fast-forward merge.

Thanks.

[rellafella]

If you're happy with the change in the src/melody/melody-extension-core/visitors/filters.js file, then it should be all good to go.
Agreed other inconsistencies and new tests can be done in a future PR.

[zackad]

LGTM.

[rellafella]

Just before you merge, I have a POC to add some better type hinting with JSdoc for utilizing the function within tests and getting the twig options combined with the prettier options. I'll push this up then it should be ready.

[rellafella]

🚀