JS-312 Create rule S1607 (no-skipped-tests): Tests should not be skipped without providing a reason
#4797
Conversation
yassin-kammoun-sonarsource
force-pushed
the
JS-312
branch
from
6290a29
to
e45eece
Compare
| */ | ||
| function hasExplanationComment(node: estree.Node): boolean { | ||
| const comments = context.sourceCode.getCommentsBefore(node); | ||
| return comments.some(comment => comment.loc!.end.line === node.loc!.start.line - 1); |
There was a problem hiding this comment.
Is a single character enough? Should there be a minimal length of explanation?
There was a problem hiding this comment.
What would you suggest as a minimal length?
There was a problem hiding this comment.
I wouldn't consider a single word to be a good enough reason. I'd say 20 characters could be fair, so that it can be a semblance of a valid sentence.
There was a problem hiding this comment.
That sounds fair, but I wonder how that would work with comments written in Asian languages such as Chinese or Japanese, which can be quite compact.
There was a problem hiding this comment.
I have been thinking, and I remember seeing quite a few times comments that just mention a Jira ticket number or a GitHub issue number.
| return jestListener(); | ||
| case dependencies.has('mocha'): | ||
| return mochaListener(); | ||
| case getPackageJsonsCount() > 0: |
There was a problem hiding this comment.
Why does this imply the nodejsListener?
There was a problem hiding this comment.
I assume that a Node.js project includes at least one package.json file so that the Node-specific listener is worth triggering. Do you think this assumption is nonsense?
There was a problem hiding this comment.
I've only ever used jest, so I'm not sure how users normally setup the nodejs-tests. It's a foreign approximation for me.
| * | ||
| * Ignoring tests with Jasmine is done by using `xit`, `xdescribe`, or `xcontext`. | ||
| */ | ||
| function jasmineListener(): Rule.RuleListener { |
There was a problem hiding this comment.
the jasmine, jest and mocha listeners follow the same convention. They only differ on the isIgnoredTest implementation. Could you merge them?
There was a problem hiding this comment.
I have mixed feelings about this because of past experiences. While it's true that the implementation is quite similar, I also feel that merging them may not be beneficial once we receive feedback from users and need to introduce framework-specific exceptions.
There was a problem hiding this comment.
Hm, well ok. I would say that's a premature separation, but I lack the experience here. Just a small fear, that there might also be improvements that will need to land in all 3 places.
yassin-kammoun-sonarsource
force-pushed
the
JS-312
branch
from
e45eece
to
a19f8bf
Compare
| }), | ||
| create(context) { | ||
| const dependencies = getDependencies(context.filename); | ||
| switch (true) { |
There was a problem hiding this comment.
oh, that's also a new one for me as well. Interesting.
| return jestListener(); | ||
| case dependencies.has('mocha'): | ||
| return mochaListener(); | ||
| case getPackageJsonsCount() > 0: |
There was a problem hiding this comment.
I've only ever used jest, so I'm not sure how users normally setup the nodejs-tests. It's a foreign approximation for me.
| * | ||
| * Ignoring tests with Jasmine is done by using `xit`, `xdescribe`, or `xcontext`. | ||
| */ | ||
| function jasmineListener(): Rule.RuleListener { |
There was a problem hiding this comment.
Hm, well ok. I would say that's a premature separation, but I lack the experience here. Just a small fear, that there might also be improvements that will need to land in all 3 places.
hashicorp-vault-sonar-prod
bot
assigned yassin-kammoun-sonarsource and unassigned yassin-kammoun-sonarsource
| @JavaScriptRule | ||
| @TypeScriptRule | ||
| @Rule(key = "S1607") | ||
| public class NoSkippedTestsCheck extends Check { |
There was a problem hiding this comment.
Do we need tests for checks of rules without parameters?
There was a problem hiding this comment.
We never did that until now. Did something change recently?
|
yassin-kammoun-sonarsource
changed the title
no-skipped-tests): Tests should not be skipped without providing a reason
RSPEC to review