feature(jest-each): Enable comments inside .each template literal table

[chrisblossom]

Summary

This PR allows comments inside of jest.each template literals.

If this PR is accepted, prettier will need to be updated not to remove comments.

Closes #8638.

Test plan

Added several tests to jest-each.

[jeysal]

Good idea and implementation LGTM as well :)

[jeysal]

What about commenting out a title line? Not sure if there's a good use case but should definitely have a test case to specify how it behaves.

[chrisblossom]

Throw on missing headngs (should be in a different PR?): 065155a.

aff5aaa#diff-a6c0398707b866880e5098b277508a6dR474

Error message could probably be improved.

[jeysal]

Maybe also a test for /* /* */ just to make sure the regex "lexer" works correctly

[chrisblossom]

I think I've added this in: fffad4d.

[mattphillips]

Hey @chrisblossom thanks for the PR!

I'm not sure I'm convinced by adding support for comments in a test template literal, would you mind expanding on your motivation more?

If one of the benefits is to disable a test row with a comment, I think it might make more sense to support an official API (same could be said for focussed tests).

Currently trailing comments are supported by using the /* comment */ syntax inside of the interpolation of a variable (and works with Prettier) e.g.

test.each`
  a              | b    | expected
  ${0}           | ${1} | ${1 /* 0 + 1 == 1 */}
  ${1 /* One */} | ${1} | ${2}
`('add $a + $b == $expected', ({ a, b, expected }) => {
  expect(a + b).toBe(expected);
});

As for comments in the title, I'd hope that the title is already expressive enough to not require a comment.

Would love to hear your thoughts on this 😄

[chrisblossom]

@mattphillips thanks for the reply! When I was initially replying I put my actual use cases in as tests and found some pretty big errors causing me to refactor how it works.

I'm not sure I'm convinced by adding support for comments in a test template literal, would you mind expanding on your motivation more?

The limited times I've used jest.each I've wanted to add comments every time for some reason or another. Whether it is to section out different code patterns, add a note at the end what the test is doing, or comment out multiple tests to work on one at a time (this is important for me personally because I extensively use Wallaby.js in combination with Jest).

test.each`
    a    | b    | expected

    // test a subset of issues
    ${0} | ${1} | ${1}

    // test another subset of issues
    ${1} | ${1} | ${2}
    ${2} | ${2} | ${4} // fix for: https://github.com/facebook/jest/pull/8717
    ${3} | ${3} | ${6} // some random note about this test

    // enable later
    // ${1} | ${1} | ${2}
    // ${1} | ${1} | ${2}
`('add $a + $b == $expected', ({ a, b, expected }) => {
    expect(a + b).toBe(expected);
});

Currently the above code ends up looking like this when I'm developing:

// ${1} | ${1} | ${2}
// ${2} | ${2} | ${4}
// ${3} | ${3} | ${6}
// ${1} | ${1} | ${2}
// ${1} | ${1} | ${2}
test.each`
    a    | b    | expected
    ${0} | ${1} | ${1}
`('add $a + $b == $expected', ({ a, b, expected }) => {
    expect(a + b).toBe(expected);
});

Often times I decide not to use jest.each because comments aren't currently supported, hence this PR.

If one of the benefits is to disable a test row with a comment, I think it might make more sense to support an official API (same could be said for focussed tests).

I think an official API would be nice, but I don't think it has to be one or the other. Comments here look natural and make code work like the rest of javascript, also have other use cases mentioned above. Even with test/describe.only I often times comment out normal tests for various reasons.

Also, at least with Webstorm and VSCode, the editor's shortcut to comment out lines work inside jest.each.

Currently trailing comments are supported by using the /* comment */ syntax inside of the interpolation of a variable (and works with Prettier) e.g.

test.each`
a              | b    | expected
${0}           | ${1} | ${1 /* 0 + 1 == 1 */}
${1 /* One */} | ${1} | ${2}
`('add $a + $b == $expected', ({ a, b, expected }) => {
expect(a + b).toBe(expected);
});

I personally do not think that is very good developer UX / doesn't look very nice.

As for comments in the title, I'd hope that the title is already expressive enough to not require a comment.

An assumption of people's code, but yes it should be.

[mattphillips]

Hey @chrisblossom thanks for the reply.

I'm still not convinced by the motivations, fundamentally when you use jest.each via the tagged template literal API you are using a string. I'm not sure we want to add coding semantics to that string that aren't valid JavaScript.

While adding // to the start of the test row looks like a comment, it is in fact not a comment. My preference is to stick to valid JavaScript and not re-implement language features from within a string template.

I think by adding an official API for focussing/skipping a row it will make developing these sorts of tests a much nicer experience.

As for the subheading comment, this probably shouldn't be achieved at this level of test but you should rather utilise a different describe block.

Alternatively you could use the Array table syntax to allow you to have comments anywhere you like.

[chrisblossom]

When I first put this PR in I didn't fully understand how tagged template literals table matched up with the data so I first tried to reconstruct the original table, then guess where comments were, then I refactored it to use the table's original array, but still guess where comments were relative to the data.

I have since refactored this to no longer guess and correctly utilize the table and data array index to match commented out data and use the same comment logic found in standard JS. This will remove any potential side effects/bugs that were potentially there previously.

I am now confident this PR is ready to be merged if accepted. I don't think I'll push anymore changes unless requested.

I'm not sure we want to add coding semantics to that string that aren't valid JavaScript.

I'm not sure I understand this comment. Comments are valid JS, I'm not proposing adding an operator that is not JS. Do you think things like jest.mock / babel-plugin-jest-hoist should be removed? Similarly those features aren't "valid javascript", but are there to increase developer UX so mocks "just work".

fundamentally when you use jest.each via the tagged template literal API you are using a string.
...
While adding // to the start of the test row looks like a comment, it is in fact not a comment.

Why does that matter that comments aren't built into tagged template literals? This is about giving better developer UX (which it seems is the very point of jest.each), which should not take into consideration the back-end implementation.

This is not a new concept. styled-components and emotion are the biggest libraries that use tagged template literals as their main API that I know of and both support JS comments (and have a plugins that enables proper syntax highlighting as well).

I think I'm the first person to open an issue here about this because I don't think jest.each is widely used.

I think by adding an official API for focussing/skipping a row it will make developing these sorts of tests a much nicer experience.

This only helps with one of several use cases here, the others are important as well.

As for the subheading comment, this probably shouldn't be achieved at this level of test but you should rather utilise a different describe block.

Isn't the point of jest.each to test multiple patterns without having to repeat the test code over and over again while increasing readability at the same time?

Maybe you personally easily understand code without comments/empty lines, but I and many others have a much harder time without them.

This feature is optional, nothing is forcing someone to add comments to thier jest.each code, but gives the option for people like me to write what I consider more-clear tests especially when we inevitably share it with coworkers/friends/open source or personally go back to it after forgetting how the code works.

Alternatively you could use the Array table syntax to allow you to have comments anywhere you like.

Yes, but there is a reason it is preferred to use the tagged template literal format. It is much nicer to read/understand especially in-combination with prettier. Better developer UX.

Also, normally I could create a package outside of jest to do this as well, but because prettier is currently programmed to remove excess characters from jest.each it is not an option.

In conclusion, I think this feature is complementary to jest.each and provides a much needed (for people such as myself) solution to several use cases mentioned above. It is opt-in and works as a developer would expect without having to come look at jest's docs or issues. I know I for one was surprised the first time I commented out a line and have jest ignore it / prettier remove it.

[jeysal]

I'm in favor of shipping this. Of course it's a bit of extra code to maintain, but it's one of those things that a user might try during development and than be happy that it just works. And if they know a bit of how JS works, they might even realize that jest-each has implemented this for them ;)

[mattphillips]

Hey guys, I'm still yet to be convinced.

The features being enabled here are:

1. Disable a row (via a comment)
2. Add comments to a cell
3. Add comments above a row (sub heading)

If we take a closer look at these features, I'm in disagree that these features need enabling via comments.

1. Disable a row - I agree this should be achievable and a new API should be proposed.

2. Add comments to a cell - This is currently supported

3. Add comments above a row - I disagree that this is a feature we want in jest-each

I acknowledge that currently styled-components and emotion support comments. The big difference with those packages is that they are application code where comments may be more relevant.

The difference with jest-each is that it is for testing, we already have many ways of describing our test with describe/test titles and column headers (for jest-each).

---

@chrisblossom I'm finding all of your arguments to be subjective to your opinion, I'd like you to please consider this and focus on tangible outcomes from this change.

The fact is, that this change brings with it a lot of maintenance overhead. To illustrate this, currently jest-each/src has 1617 lines of code (source, tests and snaps). This change raises that to 2839 lines of code - that is an increase of 75.57% just for comments, again a feature I'm not seeing the benefit from.

To go back to my point of valid JS, I don't think we should be extending the jest-each DSL to enable language level features that are partially supported and can be achieved explicitly with an API. The fact that this breaks prettier is another obvious reason to actually use valid JavaScript that is already supported and keep the DSL as thin as possible.

As I say for now I am not convinced by this at all and do not see this being merged - I'd welcome any suggestions around a potential API for skipping/focussing test rows from within a template table.

[jeysal]

Of your three points, I viewed this as a solution for the first.
But TBH I had no idea it was such a big deal in relative terms.

[chrisblossom]

it's one of those things that a user might try during development and than be happy that it just works. And if they know a bit of how JS works, they might even realize that jest-each has implemented this for them

Exactly this.

1. Disable a row - I agree this should be achievable and a new API should be proposed.

What kind of API would be acceptable to you? I first considered trying to make // make them skip, but I thought that would be unexpected by the developer / I still don't understand jest(each)'s code enough to know how to do this.

2. Add comments to a cell - This is currently supported

Yes, but it is currently hacky and not very apparent what it is for. Is the comment talking about the value inside or the whole line? Even JSX has the ability to have comments outside of values.

3. Add comments above a row - I disagree that this is a feature we want in jest-each

Again, unless I am mistaken (and so is google when you search "jest each") jest.each is here to reduce boilerplate. I fail to see why you don't see any value in this. If I have a 50 line table of tests I'd like to add some title comments and be able to separate them with an empty new line.

The big difference with those packages is that they are application code where comments may be more relevant.

Your opinion...I would find them very relevant in my code and I am sure others would as well.

I've decided multiple times against using jest.each because of the issues solved here, but otherwise it would have been a great fit.

The difference with jest-each is that it is for testing, we already have many ways of describing our test with describe/test titles and column headers (for jest-each).

The same thing could have been said about jest.each. Jest already has ways of describing tests, why add another? Well because it increases developer UX by reducing boilerplate, and so would this PR in more ways than one.

I'm finding all of your arguments to be subjective to your opinion, I'd like you to please consider this and focus on tangible outcomes from this change.

Outside of your last comment about lines added, I'd ask you to do the same. I think you are looking at this feature as it would apply to you personally and how you code.

My "arguments" are focusing around developer UX and expectations, not implementation details. This is very much a tangible outcome. I find it concerning that instead of responding to my questions / comments, you disregard/undervalue all of them by saying everything is an opinion.

Seems to me that you just fundamentally disagree with this feature because it isn't built into the language.

I am just trying to get you to understand that this PR can help me and others write more-clear code/tests, quickly focus a single test using existing IDE shortcuts, and that there is a developer expectation of them working with other popular libraries allowing them (emotion and styled components).

The fact is, that this change brings with it a lot of maintenance overhead. To illustrate this, currently jest-each/src has 1617 lines of code (source, tests and snaps). This change raises that to 2839 lines of code - that is an increase of 75.57% just for comments, again a feature I'm not seeing the benefit from.

This is completely taken out of context. The majority of the code added is from tests, which could be made into one/two tests if wanted. A lot of the tests were written because there was a lot of room for edge cases since I was guessing where comments were/where the data was. This is no longer the case with the current implementation. If wanted, I can cleanup / remove unnecessary tests. EDIT: I've since removed excess tests. It should also be noted that missing headers now throw a detailed error, which is increasing the size of the .snap as well. Now total of 2129 lines (31.66% total increase).

Funny enough that if these tests were able to be written in a jest.each, this PR significantly would help "reduce" the amount of changed lines you are referring to.

Regarding your comment here, the same thing could have been said for adding jest.each to the jest codebase. But I for one am glad it is here and thankful for you creating it.

You make me regret trying to create a complete PR that everyone could look at the tests and feel confident that it wouldn't break anyone's existing code. Have you actually taken a look at the code / tried to understand what it is doing? Feels like I am being gaslighted here...

a feature I'm not seeing the benefit from

You obviously do not see a benefit from this, but I do and I think others would too.

The fact that this breaks prettier is another obvious reason to actually use valid JavaScript that is already supported and keep the DSL as thin as possible.

It doesn't break prettier. Whoever wrote the formatting code for prettier decided to remove characters that aren't | or wrapped in ${} including excess new lines. So prettier needs to be updated to change this behavior.

The two features you mentioned 2. Add comments to a cell and 3. Add comments above a row (sub heading) currently work without issue but prettier removes them. The following is valid javascript and works as expected, but it is removed by prettier:

test.each`
    a    | b    | expected

    // test a subset of issues
    ${0} | ${1} | ${1}

    // test another subset of issues
    ${1} | ${1} | ${2}
    ${2} | ${2} | ${4} // fix for: https://github.com/facebook/jest/pull/8717
    ${3} | ${3} | ${6} // some random note about this test
`('add $a + $b == $expected', ({ a, b, expected }) => {
    expect(a + b).toBe(expected);
});

The comment markers are not needed because as you pointed out earlier, it just a string. But they are very helpful because we are trained that // are comments in JS.

I can't imagine the prettier folks will change this without the support of the jest team (although they might because as far as I know, prettier shouldn't remove code).

If the biggest concern with this PR is the 130 (with spaces and comments) lines of code added, I'm fairly certain I could significantly reduce that by only supporting single line comments and removing support for /**/. Both Webstorm and VSCode's comment shortcut inside a tagged template literal default to //.

I'm also planning on releasing this as a npm package because the implementation is not specific to Jest. (UPDATE: released as tagged-template-literal-comments) I assumed the Jest team would prefer the code to live inside the repo, but I can add the package instead, which would significantly reduce the lines changed to almost nothing...

[jeysal]

@SimenB @thymikee perhaps you have a different perspective on this, can you see this in jest-each?

[thymikee]

Oh, having the comments removed by Prettier is pretty much a blocker to me. Since a lot of JS community use it, what's the point of adding a feature that's gonna cause confusion (some may run Prettier on pre-commit hook so they wouldn't even realize)?. We should first discuss this with them and find a common ground. cc @ikatyang

On a personal note – I'm not a fan of making test.each more complicated than necessary and generally I prefer embracing the language's idiomatic style of doing things. But in this case idiomatic JS syntax is garbage (and also further breaks Prettier):

test.each`
  a | b
  0 | 1 | ${ /* comment */'' }
`

test.each is already a small DSL, so adding comments doesn't seem too big of a maintenance burden. Especially if we only supported single-line // comments. I don't like how /**/ look in the template literal provided that it's not dimmed by the editor, so I'd lean towards not supporting that.

Also, please have in mind the performance implications.

[mattphillips]

@chrisblossom I apologise if you feel I have shot this down too much - my bad!

As the author of jest-each and the primary maintainer I have to consider the cost of changes like this.

I think for future reference it would be worth raising an issue for a new feature as an RFC, this way no time is wasted on coding something that might not be accepted as a feature.

---

In regards to the DSL my preference would be to keep it thin and not put semantics around comments.

This is an API which we own, so if we do want to enable labelling (sub headings) then we should take some time and think about what that DSL looks like - for me it isn't a comment.

As for the primary focus of adding comments to disable tests - I completely disagree with this as an approach. It is not a better DX, it actually goes against Jest's DX. By adding an official API to skip/focus tests the developer experience is improved as we will include those tests in the reporting outputted at the end of a test run.

---

Going forward with this I think we should do several things:

1. Add an API for skipping and focussing rows

2. We should be more protective when parsing the headings and only deserialise the first one word after a pipe (|).

Currently any additional text will cause jest-each to fail when trying to deserialise the heading. By doing this, it will mean that you can have any additional text in the template string and the tests will still work.

3. If we do point 2, then we probably want to raise an issue in Prettier so that it doesn't strip out any additional text in the template.

If we do these three steps then users will be able to receive reporting on skipped/focussed tests in jest-each. They will also be able to add custom text to the template string without it being removed and it will not cause jest-each to fail.

This will result in minimal amount of code being added to enable the desired behaviours.

@jeysal @thymikee @chrisblossom what do you guys think about this as a way forward?

[jeysal]

I don't understand point 2, maybe an example would help.
But I agree that an integration with Jest's skipped tests feature is desirable and something I had not previously considered reviewing this.

[mattphillips]

@jeysal PR in reference to my thinking on point 2: #8766

[ikatyang]

@thymikee It shouldn't be hard to implement on the Prettier side if it only adds support for single-line comments. The reason why Prettier strips them is simply because we only expected the first TemplateElement (the non-expression part in the template string) to be meaningful.

[chrisblossom]

Sorry for the late reply...

I apologise if you feel I have shot this down too much - my bad!

No apology necessary. I know I didn't communicate very well either. I really do appreciate the time you've taken to respond, and your previous work creating jest.each.

I think for future reference it would be worth raising an issue for a new feature as an RFC, this way no time is wasted on coding something that might not be accepted as a feature.

Although I did raise an issue before submitting this PR, it wasn't very thorough. Also, I personally don't always have time to work on open source and try to take advantage of the times I do. I try and contribute back as much as I can instead of always relying on maintainers.

By adding an official API to skip/focus tests the developer experience is improved as we will include those tests in the reporting outputted at the end of a test run.

For me, this is often a worse developer experience. When using jest --watch, I typically comment all tests except the one I'm working on instead of using test.only because the massive output of skipped tests hinders TDD. Sometimes console.log statements come above test list or the terminal doesn't scroll to the bottom.

In my experience, knowing what tests are skipped is only beneficial in CI / when completing a feature and should be handled via linting.

Ignoring the feature to comment data out, I personally still think the // syntax is preferred because it is more explicit / instantly understood as a comment, and because IDEs can grey out the text:

// no comment marker necessary (https://github.com/facebook/jest/pull/8766)
test.each`
    a    | b    | expected

    test a subset of issues
    ${0} | ${1} | ${1}

    test another subset of issues
    ${1} | ${1} | ${2}
    ${2} | ${2} | ${4} fix for: https://github.com/facebook/jest/pull/8717
    ${3} | ${3} | ${6} some random note about this test
`('add $a + $b == $expected', ({ a, b, expected }) => {
    expect(a + b).toBe(expected);
});

// comment marker required (https://github.com/facebook/jest/pull/8717)
test.each`
    a    | b    | expected

    // test a subset of issues
    ${0} | ${1} | ${1}

    // test another subset of issues
    ${1} | ${1} | ${2}
    ${2} | ${2} | ${4} // fix for: https://github.com/facebook/jest/pull/8717
    ${3} | ${3} | ${6} // some random note about this test
`('add $a + $b == $expected', ({ a, b, expected }) => {
    expect(a + b).toBe(expected);
});

test.each is already a small DSL, so adding comments doesn't seem too big of a maintenance burden. Especially if we only supported single-line // comments. I don't like how /**/ look in the template literal provided that it's not dimmed by the editor, so I'd lean towards not supporting that.

If wanted, I could remove /**/ support from the PR, although it doesn't remove as much code as I was originally expecting. Seems like this is better for prettier as well according to @ikatyang.

FYI, editors can (and do) dim comments inside template literals (See: emotion and styled-components).

Also, please have in mind the performance implications.

Definitely something to consider, although in practice I don't see this being an issue. I've sped this up quite a bit with a package I'll be releasing soon that uses regexes instead splitting all characters and looping through them.

1. Add an API for skipping and focussing rows

I think this is a great idea regardless if this PR is merged or not. What are some ideas on how this would look?

2. We should be more protective when parsing the headings and only deserialise the first one word after a pipe (|).
3. If we do point 2, then we probably want to raise an issue in Prettier so that it doesn't strip out any additional text in the template.

Although I think this PR solves this in a better, cleaner way, this would at least allow users to add their own comments / additional new lines. Also, commenting out values could then be solved outside of jest as a third-party library.

[jeysal]

this would at least allow users to add their own comments / additional new lines

Oh wow, I only just realized that Matt's PR that ignores everything jest-each also implicitly allows comments by ... ignoring them. So far I just though this was a 'btw' cleanup unrelated to your problem ^^

[SimenB]

What's the status here? Is it still needed after #8766 landed in v27?

[github-actions]

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

[github-actions]

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.