Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW RULE - Require test case titles to have a hashtag #605

Closed
wants to merge 5 commits into from
Closed

NEW RULE - Require test case titles to have a hashtag #605

wants to merge 5 commits into from

Conversation

MutterPedro
Copy link

Hello all, I got an idea about a new rule, as the title says, to require the title of the it() blocks to have at least one hashtag (e.g. this is a #unit test). In the projects that I work, I always try to keep consistency and give a hashtag to all of my tests, to make it easy for me to choose which kind of tests I want to run, or maybe the context of the tests that I want to be run. But it is fairly easy to forget to put the hashtag on every test, especially when you create a lot of tests as on the TDD approach.

Also, I added an option to set the only allowed hashtags, to help to keep a more strict consistency between the tests:

{
    "allowedHasgtags": ["unit", "integration", "e2e"]
}

As I am a little forgot guy, I would love to have such a rule to help me when I am writing my test cases. Hope you guys like it 馃槂

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 20, 2020

Thanks for this, but I'm thinking this would be better supported using an option on valid-title, as described in #233 :)

@MutterPedro
Copy link
Author

Ok, I got your point @G-Rath, but that would be a much more general case solution. Think with me: when a new developer starts to help my team in a project that has that rule for the tests. He writes down the test but didn't use a mandatory tag, and the IDE is complaining that the title is invalid, he would think "Why is that?", and lost productivity figuring out what is wrong.

Also, that option to valid-title could be ambiguous. My tests must contain all the strings defined in the mustMatch option, or just one of those is enough? I wouldn't know the answer the first time I'd see it.

Anyway, I would like a third opinion about this new rule that I am proposing before closing this PR.

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 21, 2020

that would be a much more general case solution

Yes, which is the point: this rule seems highly specific to your personal way of development. I've never seen tests in any project (across languages, not just in the jest/js community) use hashtags like this; typically scoping is done using folders.

That's not to say it's wrong or invalid, but we have to maintain theses rules, so taking on one like this that the community won't use is of little gain vs supporting a more generic solution, especially given that you can publish your own plugin with rules, or source rules locally (You can check out this repo as an example of project that uses some custom local rules).

As such:

lost productivity figuring out what is wrong

I feel you're going to have this either way: both rules have messages shown by eslint that explain what the problem is, so in both situations (using your rule vs mustMatch) either the developer sees & understands the messages, or they spend time exploring in which case you'll have this problem with all the rules since that's why we have the messages.

As I said above this is a pattern I've never seen in testing before, so I imagine that developers will require some onboarding anyway, as to understand the intentions behind the hashtags, asking questions such as "why?", "how do I use them?", "how do I know what hashtag to use?", etc

Either way, this productivity loss is a one-time cost that can be easily mitigated by including it in contributing documentation.

Also, that option to valid-title could be ambiguous

I disagree, as the property takes a Regular Expression, which is included in the rule message, so even if you don't check the configuration to find the mustMatch value, when you hit it you'll see that it's a Regular Expression instead of a string and so treat it like any other: parse it to figure out what it matches.


Overall I feel the generic solution is more likely to be useful for the community.

@MutterPedro
Copy link
Author

I agree with you on most of the points. But I still think that the 2 rules can live together, mustMatch is a great feature too I will use it I am almost sure. And I am pretty sure that I would not the only one who would use the proposed rule, I forget to mention, but this practice of tagging the tests, is a best practice related to JS testing described here. I know this repo is not the source of all truths, but those advices helped me a lot, and I missed very hard an eslint rule to help me keep this tag practice easy to maintain and easy to be adopted by my teammates.

That's not to say it's wrong or invalid, but we have to maintain theses rules, so taking on one like this that the community won't use is of little gain vs supporting a more generic solution, especially given that you can publish your own plugin with rules, or source rules locally (You can check out this repo as an example of project that uses some custom local rules).

Thanks for this advice. It is also a very good approach to deal with very specialized rules, I will consider its adoption in my next enterprise project for sure 馃憤.

And if you guys are needing help with the projects, and are considering new members, I would be very happy to help! 馃槃

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 22, 2020

I've updated #608 so the rule will now support both mustMatch & mustNotMatch options, which allows you to enforce hashtags in full.

See this test for the RegExps to use.

For the sake of completeness, here is how you could configure the rule so that it allows titles both either without hashtags, or with only known hashtags:

// .eslintrc.js
const hashtags = ['unit', 'e2e'].join('|');

module.exports = {
  plugins: ['jest'],
  rules: {
    'jest/valid-title': [
      'error',
      {
        mustNotMatch: new RegExp(`(?:#(?!${hashtags}))\w+`, 'u').source,
        mustMatch: new RegExp(`^[^#]+$|(?:#(?:${hashtags}))`, 'u').source,
      },
    ],
  },
};

with the above "my title" & "my title #unit" but not "my title #unit #jest4life".

@MutterPedro
Copy link
Author

I am happy for inspiring a new option to a rule with my idea 馃槃. But a little bit sad about my rule not being added 馃槥 .

However, I am happy to help in any way:tada:

@G-Rath G-Rath closed this in #608 Jul 5, 2020
@github-actions
Copy link

github-actions bot commented Jul 5, 2020

馃帀 This issue has been resolved in version 23.18.0 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants