-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New handling of .each calls breaks no-identical-title #836
Comments
Hey, colleague of @DanielSWolf here. The following should be a minimal example that shows the describe('Test', () => {
it.each(['a', 'b'])('%s', replacement => {
console.log(replacement);
});
it.each(['c', 'd'])('%s', replacement => {
console.log(replacement);
});
}); |
ah thank you - that's to be expected as we don't do any checking for replacement placeholders. That's next up on my todo list now that I've gotten the We've already got a couple of issues open for this, but this might not be a direct duplicate as I think they're more for new rules that check you've got a placeholder in your title rather than modifying duplicate title. I'll handle adjusting the issue as needed to represent the feature request when I've got a few minutes spare :) |
So I've been thinking about this for a bit and can't decide on what we should consider an identical title or not: I feel that if you've got two tests next to each other that have a title like That could be extended to when you also just have a replacement (i.e Maybe the answer here is to provide a (while I'm fine with not having a perfect answer, my concern here is that you could argue changing this logic later should be considered a breaking change) |
We have a number of cases where the format strings are identical, but the format of the tabular data is not. For technical reasons, some cases may require a different data format while still belonging to the same logical test. So we actually want the titles to look identical (to stress that these cases belong together), but are forced to split them into several groups for technical reasons.
From our point of view, the breaking change was introduced with #814. Before, multiple Might it be feasible to allow |
Sorry but that won't be happening - it was always a bug that we weren't handling
But that's my point: you should still be able to give those tests unique titles in that case, since there is a difference in the data - be it in the
|
just to be clear (and give myself an out 😅): we won't reverting the implementation, but that doesn't mean we won't add support for changing the rule behaviour as an option nor fine-tuning the checking (which is most likely what needs to happen here) - I'm currently angling towards this being the default because it aligns with how the jest docs describe it, but I'm wanting to understand more about the possible alternatives and why you can/cannot take them :) |
I'm running into the same issue. My tests are setup like the following: describe.each(FIELD_USER_ROLES)('when the user role is %s', (userRole) => {
let ctx;
beforeAll(async () => {
ctx = await setupFieldUserTestContext();
});
});
describe.each(OFFICE_USER_ROLES)('when the user role is %s', (userRole) => {
let ctx;
beforeAll(async () => {
ctx = await setupOfficeUserTestContext();
});
}); As you can see, these tests cannot be combined due to the setup needed for each set being different. Sure, for now I can change the title to be something like 👍 for |
🎉 This issue has been resolved in version 24.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This issue has been resolved in version 25.0.0-next.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Earlier this year, issue #786 described that
.each
blocks with identical format strings erroneously causedno-identical-title
messages. This was fixed in PR #788.It seems that with the merge of PR #814, this faulty behavior happens again.
The text was updated successfully, but these errors were encountered: