-
Notifications
You must be signed in to change notification settings - Fork 29
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Create rule S1607: Tests should not be skipped without providing a re…
…ason (#4218)
- Loading branch information
1 parent
e5ae27a
commit 21bf3f4
Showing
3 changed files
with
144 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"title": "Tests should not be skipped without providing a reason", | ||
"quickfix": "infeasible" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,137 @@ | ||
== Why is this an issue? | ||
|
||
Disabling unit tests lead to a lack of visibility into why tests are ignored, a decline in code quality as underlying problems remain unaddressed, and an increased maintenance burden due to the accumulation of disabled tests. It can also create a false sense of security about the stability of the codebase and pose challenges for new developers who may lack the context to understand why tests were disabled. Proper documentation and clear reasons for disabling tests are essential to ensure they are revisited and re-enabled once the issues are resolved. | ||
|
||
This rule raises an issue when a test construct from Jasmine, Jest, Mocha, or Node.js Test Runner is disabled without providing an explanation. It relies on the presence of a +package.json+ file and looks at the dependencies to determine which testing framework is used. | ||
|
||
== How to fix it in Jasmine | ||
|
||
A comment should be added before the line of the unit test explaining why the test was disabled. Alternatively, if the test is no longer relevant, it should be removed entirely. | ||
|
||
=== Code examples | ||
|
||
==== Noncompliant code example | ||
|
||
[source,javascript,diff-id=1,diff-type=noncompliant] | ||
---- | ||
describe('foo', function() { | ||
xit('should do something', function(done) { // Noncompliant | ||
done(); | ||
}); | ||
}); | ||
---- | ||
|
||
==== Compliant solution | ||
|
||
[source,javascript,diff-id=1,diff-type=compliant] | ||
---- | ||
describe('foo', function() { | ||
// Reason: There is a bug in the code | ||
xit('should do something', function(done) { // Compliant | ||
done(); | ||
}); | ||
}); | ||
---- | ||
|
||
== How to fix it in Jest | ||
|
||
A comment should be added before the line of the unit test explaining why the test was disabled. Alternatively, if the test is no longer relevant, it should be removed entirely. | ||
|
||
=== Code examples | ||
|
||
==== Noncompliant code example | ||
|
||
[source,javascript,diff-id=2,diff-type=noncompliant] | ||
---- | ||
describe('foo', function() { | ||
test.skip('should do something', function(done) { // Noncompliant | ||
done(); | ||
}); | ||
}); | ||
---- | ||
|
||
==== Compliant solution | ||
|
||
[source,javascript,diff-id=2,diff-type=compliant] | ||
---- | ||
describe('foo', function() { | ||
// Reason: There is a bug in the code | ||
test.skip('should do something', function(done) { // Compliant | ||
done(); | ||
}); | ||
}); | ||
---- | ||
|
||
== How to fix it in Mocha | ||
|
||
A comment should be added before the line of the unit test explaining why the test was disabled. Alternatively, if the test is no longer relevant, it should be removed entirely. | ||
|
||
=== Code examples | ||
|
||
==== Noncompliant code example | ||
|
||
[source,javascript,diff-id=3,diff-type=noncompliant] | ||
---- | ||
describe('foo', function() { | ||
it.skip('should do something', function(done) { // Noncompliant | ||
done(); | ||
}); | ||
}); | ||
---- | ||
|
||
==== Compliant solution | ||
|
||
[source,javascript,diff-id=3,diff-type=compliant] | ||
---- | ||
describe('foo', function() { | ||
// Reason: There is a bug in the code | ||
it.skip('should do something', function(done) { // Compliant | ||
done(); | ||
}); | ||
}); | ||
---- | ||
|
||
== How to fix it in Node.js | ||
|
||
A non-empty string literal should be passed to the skip options or as an argument to the call to skip (``++{ skip: 'reason' }++``) on the test context (``++t.skip('reason')++``), explaining why the test was disabled. | ||
|
||
=== Code examples | ||
|
||
==== Noncompliant code example | ||
|
||
[source,javascript,diff-id=4,diff-type=noncompliant] | ||
---- | ||
const test = require('node:test'); | ||
test('should do something', { skip: true }, function(t) { // Noncompliant | ||
t.assert.ok(true); | ||
}); | ||
test('should do something', function(t) { | ||
t.skip(); // Noncompliant | ||
}); | ||
---- | ||
|
||
==== Compliant solution | ||
|
||
[source,javascript,diff-id=4,diff-type=compliant] | ||
---- | ||
const test = require('node:test'); | ||
test('should do something', { skip: 'There is a bug in the code' }, function(t) { // Compliant | ||
t.assert.ok(true); | ||
}); | ||
test('should do something', function(t) { | ||
t.skip('There is a bug in the code'); // Compliant | ||
}); | ||
---- | ||
|
||
== Resources | ||
|
||
=== Documentation | ||
|
||
* Jasmine Documentation - https://jasmine.github.io/api/3.0/global.html#xit[xit] | ||
* Jest Documentation - https://jestjs.io/docs/api#testskipname-fn[test.skip] | ||
* Mocha Documentation - https://mochajs.org/#inclusive-tests[Inclusive tests] | ||
* Node.js Documentation - https://nodejs.org/docs/latest/api/test.html#skipping-tests[Skipping tests] |