add integration test & fixtures setup for more confidence

[JaKXz]

@RunDevelopment I thought about wanting to keep the tests I had written in PrismJS#2927 passing in their current form without removing the workaround code I had for the plugins, but then I thought I could take it a step further and have "end to end" tests for the entire Prism lifecycle...

Curious what you would think of this, if it would be worth exploring separately? I would love to have something like this to have full confidence on top of all the great unit tests Prism already has; however, this rabbit hole is kind of endless in terms of combinations of plugins and languages so it could get out of hand...

I think it would be reasonable to test one language at a time like this, with a folder structure in the e2e folder that made it clear what plugins and other components you wanted to test for full confidence. What do you think?

[RunDevelopment]

I don't think we need that much new test setup to do E2E tests. Prism already has something similar. I.e. this markup test tests a plugin included in markup that adds an attribute to all entities. So we can do E2E testing already but the test format is really inconvenient.

My idea would be to add a "new" test format. The "new" format would be exactly the same as the current .test files but instead of the JSON of the token stream, they store the produced HTML (the HTML that is currently in your Jest snapshot). This should be pretty easy to implement.

This would also work for you, right?

[JaKXz]

@RunDevelopment agreed... I think however that we're missing an opportunity to not re-invent this wheel and move away from Prism's handrolled format. I wonder if the extra maintenance work on the custom runner is worth it when we could move to the new community standard that is easy to work with.

When I first saw the .test files, I thought of jest snapshots right away... I realized that Prism does pre-date modern snapshots so I recognize the necessity. Going forward I think this simplifies new contributions because it is such a widely understood /common pattern and we don't have to worry about the details of the parser implementation to get our tests exactly right. There's also more powerful tooling like the html serializer available.

Additionally:

• the --update feature is built right in
• Works very well with mocha’s watch flag
• don't have to worry about line endings which makes snapshots easier to diff in local git clients

[JaKXz]

ava is an alternative test runner that has powerful diffs and snapshots built in as well, but I didn't suggest / try it here because it would be a bigger migration/more setup work to switch runners or to have two runners side by side.

I also like the idea of keeping the existing tokenizer tests (and only adding new e2e tests as needed) so staying in the mocha/chai world is better 👍🏽

[RunDevelopment]

I get what you mean. Snapshots are pretty much an industry standard nowadays that make it easy to write tests.

However, our format has a few advantages over snapshots specifically in our domain:

1. Escaping. With our test format, contributors don't have to worry about escaping certain characters at all. One can just copy-paste code. Of course, String.raw exists but there still certain text sequences that have to be escaped (e.g. ${).
2. Easy extraction of code. We also use the code snippets in the test files to analyze our regexes. This is trivial to do with our test format. I'm sure that we could also just export snippets but it's not as convenient.
3. Ease of use. Creating a test file is as easy as creating a new file and pasting in text.
4. Single file. Snapshots are typically stored in a separate file. Having everything in a single file is more orderly IMO but that's just personal preference.

don't have to worry about line endings which makes snapshots easier to diff in local git clients

How would snapshots solve this?

[JaKXz]

don't have to worry about line endings which makes snapshots easier to diff in local git clients

How would snapshots solve this?

We wouldn't have to use binary for the .snap files so it's easy to see what has changed in an app like GitHub Desktop or something. e.g. when I am browsing through my changes in my visual tool and I see this:

It's pretty hard to work with, but with snapshots we only worry about passing the right string to the function subject under test, and we get a format that's easy to read (and shows up in GitHub desktop as plain text as a bonus). I think this is a fundamental point: this makes the tests closer to the end-user experience, i.e.

1. I load up Prism and plugins I want
2. I pass in my code block
3. I get an HTML string with all the tokens I expect.

[RunDevelopment]

don't have to worry about line endings which makes snapshots easier to diff in local git clients

How would snapshots solve this?

We wouldn't have to use binary for the .snap files so it's easy to see what has changed in an app like GitHub Desktop or something. e.g. when I am browsing through my changes in my visual tool and I see this:

Sorry, that not what I was referring to. I meant the "don't have to worry about line endings" part. I don't see how any snapshot solution would let us choose the line ends if the code is stored in JS files.

Also, I think that the problem with GitHub desktop is something GitHub desktop should fix. GitHub and VSCode (the tools I use) have no problem with this.

this makes the tests closer to the end-user experience, i.e.

1. I load up Prism and plugins I want
2. I pass in my code block
3. I get an HTML string with all the tokens I expect.

This whole test suite was designed so we don't have to do steps 1 and 2 for every test...

Also, we want most tests to only consider the token stream and not the HTML string. The token stream is a lot more compact and readable. Humans (e.g. I) have to be able to understand the token stream and that's not easy by going through a bunch of HTML.

[JaKXz]

This whole test suite was designed so we don't have to do steps 1 and 2 for every test...
Also, we want most tests to only consider the token stream and not the HTML string.

I agree, like I said here I think keeping the tokenizer tests around for quickly verifying the basic token stream is a totally valid use case.

My point is that this style of e2e test makes it easy to contribute/verify expected behaviour for the entire system, and you would only add these tests as needed, when you want to verify that the entire produced HTML string is what you expect and are willing to check it in its entirety. This will of course be the exception but I think it really helps those who are unfamiliar with the codebase see how everything comes together and makes it so that it's easy to contribute without having to worry about breaking things for others who have put the time in to write these kinds of tests.

EDIT:

if the code is stored in JS files

just to state it explicitly, the produced html code is not stored in JS files, if that's what you were referring to? :)

[joshgoebel]

Also, I think that the problem with GitHub desktop is something GitHub desktop should fix. GitHub and VSCode (the tools I use) have no problem with this.

Doesn't git diff itself refuse to show diffs for binary files? I use git diff all the time...

[RunDevelopment]

This whole test suite was designed so we don't have to do steps 1 and 2 for every test...
Also, we want most tests to only consider the token stream and not the HTML string.

I agree, like I said here I think keeping the tokenizer tests around for quickly verifying the basic token stream is a totally valid use case.

Ahh sorry, I missed that part. I thought you wanted to replace all language tests with snapshots.

But this opens another question: Do you intend this to replace the current .js tests (example)? The .js tests fulfill the same purpose, as far as I can see (here is the code that currently runs the .js tests).

If possible, I would like to replace the current .js tests (either with this or something else).

if the code is stored in JS files

just to state it explicitly, the produced html code is not stored in JS files, if that's what you were referring to? :)

No, I was talking about the input code.

Escaping is annoying but there's also a more pressing issue: We have to able to get the input code for the analyzer detecting exponential backtracking. The input code in these test files might be the only one covering some code path that uses a regex with exponential backtracking. If we miss even one of them, it's another CVE.

Being able to access the input code is also important for another PR that hasn't been merged yet. It will essentially do code coverage based on our language tests. (Sadly, Istanbul doesn't really work with Prism. Istanbul doesn't know which source files are being executed because of prism-loader.)

Could there be an easy way for other tests to get the input code?

Also, I think that the problem with GitHub desktop is something GitHub desktop should fix. GitHub and VSCode (the tools I use) have no problem with this.

Doesn't git diff itself refuse to show diffs for binary files? I use git diff all the time...

Oof, you're right. Can't fault GitHub desktop for that one.

[JaKXz]

If possible, I would like to replace the current .js tests (either with this or something else).

@RunDevelopment I would happily update all the other .js tests to this format in a separate PR(s) if we are good to move forward with this -- I really think it's so much easier to work with and so much less setup than the existing .js test runner or the .testhtml approach.

To address the issue of being able to access the input code (that's a great consideration, thank you for pointing it out as I wasn't aware):

I think we could use fixtures files which export arrays of examples and generate the tests that way. We can then share the fixtures with the other instrumentation that needs it. I'll put up another commit right here to show what I mean.

[JaKXz]

@RunDevelopment I've pushed a couple commits here, one with the proposed shareable fixtures and a couple more to fix up the tokens and the tokenizer hook. I think the tip of this branch will be what I will be going forward with.

In addition to the reasoning provided in this comment I think the most valuable thing about snapshot testing here is the diffable html formatting that makes parsing the snapshots while working on a failing set of cases much easier. I found it very difficult to parse the formatting in the existing .js test partially because it is arbitrary. With snapshots + jest-serializer-html, each outputted span is consistently formatted and finding out what classnames are applied to what piece of code is easy.

I think the use of template strings in the shareable fixtures addresses concerns 1. and 2. mentioned here. I think the other points do come down to personal preference, but again, I think since the userbase for snapshots is pretty high I'd say the majority would be in favour of snapshots over the custom format. Since I am suggesting this format for tests on an as needed basis only, to help with more complex cases like this one e.g., I think the lower maintenance overhead is so much better going forward.

[RunDevelopment]

A huge plus for the diffable HTML from me.

I think the use of template strings in the shareable fixtures addresses concerns 1. and 2. mentioned here. I think the other points do come down to personal preference

Agreed.

I think since the userbase for snapshots is pretty high I'd say the majority would be in favour of snapshots over the custom format.

Snapshots are popular because they are a very simple solution for a problem - storing the expected result of a test. It doesn't matter how you do it as long as it's convenient and simple to use for developers.

---

I've been thinking about this yesterday and I want to hear your thoughts.

First of all, I think a category of E2E test makes sense for Prism but:

1. We are already sorta doing E2E testing on our website. Every plugin page has an example section where (hopefully) every feature of the plugin is presented and thereby also tested. This is by far not a replacement for more synthetic automated tests but it's as close to end-to-end as we can get.

2. I don't think the tests this PR added can even be classified as E2E tests.

   Prism.highlight is still a relatively low-level function. For GraphQL+Highlight Keywords, this function covers all of their behavior but two components interacting with each other can hardly be called end to end. From what I see, they would probably be classified as integration tests.

   Right now, the GraphQL+Highlight Keywords do nothing special compared to other language tests. We have tests of two components interacting with each other. We have tests of HTML output. We have language+plugin tests (some languages bring their own plugins and/or use the plugins of other languages).

   I just don't see why the tests in this PR deserve their own category. Maybe I misunderstood what you meant with E2E but from what I see, they are just another language test.

Secondly, since the test data is now in fixture files, wouldn't it make sense to simply change our current .js tests to use snapshots instead of creating a new category of tests? From what I see, it could be as simple as changing a few lines in runTestsWithHooks, though it might be more complicated because of where snapshots are stored.

[JaKXz]

From what I see, they would probably be classified as integration tests. Right now, the GraphQL+Highlight Keywords do nothing special compared to other language tests.

That's fair. I'm not too hung up on what they're called, it was just important to me to get all the components I'm using in this example wired up in an easy to work with (fast feedback with mocha's --watch working properly instead of having to use entr, diffable html, etc) setting.

I just don't see why the tests in this PR deserve their own category. Maybe I misunderstood what you meant with E2E but from what I see, they are just another language test.

The reason for this is [somewhat]:

it might be more complicated because of where snapshots are stored.

.snap files are generated based on the name of the -test.js file that the assertion is made from so it was easier to separate it out here. I didn't want to change runTestsWithHooks to call toMatchSnapshot() because it would have been one massive .snap file that would have grown every time we add a new test case. With the organization I'm proposing the .snap files will be smaller and isolated to the integration between components being tested.

I would be interested in a way that tests all hooks registered...so that it is truly "end to end". In a separate PR though.

We are already sorta doing E2E testing on our website. Every plugin page has an example section where (hopefully) every feature of the plugin is presented and thereby also tested.

Are we? https://prismjs.com/examples/prism-graphql.html doesn't render the code properly with any theme.

[RunDevelopment]

.snap files are generated based on the name of the -test.js file

Yup, that's a problem. Ideally, we could have a file structure like this:

tests/
  languages/
    my-language+dependencies/
      __snapshots__/ # or some other name
        my-test.snap
      my-test.js

Is there really no way for us to control where the snap file lives?

With the organization I'm proposing the .snap files will be smaller and isolated to the integration between components being tested.

But this organization also divides our tests into two kinds even though it's not strictly necessary. We will also have to create a new file for each fixture to run it, right?

Also, these HTML tests are not exclusively used to test "the integration between components", they are also used by single components.

I would be interested in a way that tests all hooks registered...so that it is truly "end to end"

runTestsWithHooks gets pretty close. Going farther would require a virtual DOM. Not a problem but also not necessary for languages. AFAIK, we have no languages with plugins that use hooks that use the DOM.

Are we? https://prismjs.com/examples/prism-graphql.html doesn't render the code properly with any theme.

All files in the examples/ directory are HTML templates can be viewed on the examples page. Pretty inconvenient, I know...

I was talking about our plugin pages. Each plugin has a page with examples. These examples typically showcase and test all features. Example.

Languages aren't E2E-tested because it's not typically necessary. As I said, they don't use the DOM, only some even use hooks, and the ones that do use hooks, typically only use the hooks within Prism.highlight.

[JaKXz]

Unfortunately, no there is no way that I've found to determine where the snap file lives or to create a new snap file for each assertion. I think the tradeoff is worth it, even though the tests have to be "divided". I think it simplifies the original test suite, being able to just run the feature.test files as the unit tests for the tokenizer. Creating a new file for each combination (e.g. graphql + highlight keywords), as needed to help fix a bug or add features, is a reasonable way to work and help someone approach the system.

I think the rest of your comments are something we can discuss in follow up PRs. I've renamed the e2e misnomer now :) -- do you think this is an acceptable pattern & tradeoff for ease of use?

Also, these HTML tests are not exclusively used to test "the integration between components", they are also used by single components.

Could you elaborate on this? Do you mean the other .js tests that exist in the repo?

[RunDevelopment]

Unfortunately, no there is no way that I've found to determine where the snap file lives or to create a new snap file for each assertion.

But that means that to add a single test, I now have to create 2 files.

Maybe an example: Let's say I have a language like JavaScript. I fixed a bug, add one test using your system, and make a PR - PR1. Then, while PR1 is still open (review takes a while), I add a new feature to JS, add a test for that feature using your system, and make a PR - PR2.
How many and which test files do I have to create/change for PR1 and then for PR2. Will changes in PR2 conflict (as in merge conflicts) with the changes in PR1 (assuming no conflicts in JS itself) and how easy is it to resolve these conflicts?

I think it simplifies the original test suite,

being able to just run the feature.test files as the unit tests for the tokenizer.

These tests aren't supposed to test the tokenizer but the languages. We have tests specifically for the tokenization process as well.

Creating a new file for each combination (e.g. graphql + highlight keywords), as needed to help fix a bug or add features, is a reasonable way to work and help someone approach the system.

But this is also true for the current system, is it not?

Also, these HTML tests are not exclusively used to test "the integration between components", they are also used by single components.

Could you elaborate on this? Do you mean the other .js tests that exist in the repo?

Yes, we also have .js tests that just test aliases (which aren't visible in .test file (JSON tests)) of a single component, a unit test so to say. The only reason why this is a rare sight is that current .js tests are a pain to use. I've written hundreds of .test files but only a single .js test, IIRC.

[JaKXz]

The only reason why this is a rare sight is that current .js tests are a pain to use. I've written hundreds of .test files but only a single .js test, IIRC.

That's what I'm suggesting can be fixed (replaced) with this. Now it's very easy to have a test that is very easy to work with. The effort of adding 2 files per test file (or combination, say javascript + highlight-keywords) is only because wanted to share the fixtures, but if we don't want to add multiple files for a test, we don't have to do that. It would also only be for the first time you wanted to add to a test, otherwise you would just be adding your fixtures to the languages/:language/fixtures array. The setup is easy enough since the chai helper and prism-loader files do most of the work so you can focus on the samples you want to verify. It's very common to have a bit of shared setup at the start of every test file, and as I said before, I think the few lines repeated is a fine tradeoff for a much better DX.

But this is also true for the current system, is it not?

I don't think it's as easy to use as this, especially if you want to test the "final" (Prism.highlighted -- we can talk about adding more of the hooks later) html. i.e. I think running npm run test:integration -- --watch is so much faster & easier to work with that it will become the preferred dev process. You can't do that with the language .test files as they are now, you have to use entr or some other filesystem watcher which isn't always reliable.

Resolving merge conflicts in snapshot files is usually (>95% of the time) easy. The process is handled for you by simply rebasing PR1 on PR2 (or PR2 on PR1 (master) if PR1 is merged first) and running --update.

[JaKXz]

I opened PrismJS#2939 so that we can discuss these changes separately from the language changes underneath. :)