JS-312 Improve S1607 (no-skipped-tests): Allow explanation comments…

… adjacent to the disabled test (#4804)

packages/jsts/src/rules/S1607/fixtures/jasmine/cb.fixture.js

@@ -8,36 +8,56 @@ describe('foo', function() {
 
 describe('foo', function() {
   // Reason: There is a bug in the code
-  xit('should do something', function(done) { // Compliant
+  xit('should do something', function(done) {
     done();
   });
 });
 
+describe('foo', function() {
+  xit('should do something', function(done) { // Reason: There is a bug in the code
+    done();
+  });
+});
 
 describe('foo', function() {
-  xit('should do something', function(done) { // Noncompliant {{Remove this unit test or explain why it is ignored.}}
+  xit('should do something', function(done) {
+    // Reason: There is a bug in the code
+    done();
+  });
+});
+
+// Noncompliant@32 {{Remove this unit test or explain why it is ignored.}}
+
+describe('foo', function() {
+  xit('should do something', function(done) {
 //^^^
     done();
   });
 });
 
+// Noncompliant@41
+
 describe('foo', function() {
-  xcontext('foo', function() { // Noncompliant
+  xcontext('foo', function() {
     it('should do something', function(done) {
       done();
     });
   });
 });
 
-xdescribe('foo', function() { // Noncompliant
+// Noncompliant@50
+
+xdescribe('foo', function() {
   it('should do something', function(done) {
     done();
   });
 });
 
+// Noncompliant@60
+
 describe('foo', function() {
   //
-  xit('should do something', function(done) { // Noncompliant
+  xit('should do something', function(done) {
     done();
   });
 });

packages/jsts/src/rules/S1607/fixtures/jest/cb.fixture.js

@@ -8,44 +8,55 @@ describe('foo', function() {
 
 describe('foo', function() {
   // Reason: There is a bug in the code
-  it.skip('should do something', function(done) { // Compliant
+  it.skip('should do something', function(done) {
     done();
   });
 });
 
+// Noncompliant@19 {{Remove this unit test or explain why it is ignored.}}
 
 describe('foo', function() {
-  it.skip('should do something', function(done) { // Noncompliant {{Remove this unit test or explain why it is ignored.}}
+  it.skip('should do something', function(done) {
 //^^^^^^^
     done();
   });
 });
 
+// Noncompliant@28
+
 describe('foo', function() {
-  test.skip('should do something', function(done) { // Noncompliant
+  test.skip('should do something', function(done) {
     done();
   });
 });
 
-describe.skip('foo', function() { // Noncompliant
+// Noncompliant@35
+
+describe.skip('foo', function() {
   it('should do something', function(done) {
     done();
   });
 });
 
+// Noncompliant@44
+
 describe('foo', function() {
-  xit('should do something', function(done) { // Noncompliant
+  xit('should do something', function(done) {
     done();
   });
 });
 
+// Noncompliant@52
+
 describe('foo', function() {
-  xtest('should do something', function(done) { // Noncompliant
+  xtest('should do something', function(done) {
     done();
   });
 });
 
-xdescribe('foo', function() { // Noncompliant
+// Noncompliant@59
+
+xdescribe('foo', function() {
   it('should do something', function(done) {
     done();
   });

packages/jsts/src/rules/S1607/fixtures/mocha/cb.fixture.js

@@ -8,28 +8,33 @@ describe('foo', function() {
 
 describe('foo', function() {
   // Reason: There is a bug in the code
-  it.skip('should do something', function(done) { // Compliant
+  it.skip('should do something', function(done) {
     done();
   });
 });
 
+// Noncompliant@19 {{Remove this unit test or explain why it is ignored.}}
 
 describe('foo', function() {
-  it.skip('should do something', function(done) { // Noncompliant {{Remove this unit test or explain why it is ignored.}}
+  it.skip('should do something', function(done) {
 //^^^^^^^
     done();
   });
 });
 
+// Noncompliant@28
+
 describe('foo', function() {
-  context.skip('foo', function() { // Noncompliant
+  context.skip('foo', function() {
     it('should do something', function(done) {
       done();
     });
   });
 });
 
-describe.skip('foo', function() { // Noncompliant
+// Noncompliant@37
+
+describe.skip('foo', function() {
   it('should do something', function(done) {
     done();
   });

packages/jsts/src/rules/S1607/rule.ts

@@ -234,14 +234,21 @@ export const rule: Rule.RuleModule = {
     }
 
     /**
-     * Checks if the node denoting a test has an explanation comment on the line above.
+     * Checks if the node denoting a test has an adjacent explanation comment.
      */
-    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 && comment.value.trim().length > 0,
-      );
+    function hasExplanationComment(node: estree.CallExpression) {
+      function isAdjacent(comment: estree.Comment, node: estree.Node) {
+        const commentLine = comment.loc!.end.line;
+        const nodeLine = node.loc!.start.line;
+        return Math.abs(commentLine - nodeLine) <= 1;
+      }
+
+      function hasContent(comment: estree.Comment) {
+        return /\p{L}/u.test(comment.value.trim());
+      }
+
+      const comments = context.sourceCode.getAllComments();
+      return comments.some(comment => isAdjacent(comment, node) && hasContent(comment));
     }
   },
 };

sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1607.html

@@ -6,8 +6,8 @@ <h2>Why is this an issue?</h2>
 <p>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.</p>
 <h2>How to fix it in Jasmine</h2>
-<p>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.</p>
+<p>A comment should be added before, on, or after 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.</p>
 <h3>Code examples</h3>
 <h4>Noncompliant code example</h4>
 <pre data-diff-id="1" data-diff-type="noncompliant">
@@ -27,8 +27,8 @@ <h4>Compliant solution</h4>
 });
 </pre>
 <h2>How to fix it in Jest</h2>
-<p>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.</p>
+<p>A comment should be added before, on, or after 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.</p>
 <h3>Code examples</h3>
 <h4>Noncompliant code example</h4>
 <pre data-diff-id="2" data-diff-type="noncompliant">
@@ -48,8 +48,8 @@ <h4>Compliant solution</h4>
 });
 </pre>
 <h2>How to fix it in Mocha</h2>
-<p>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.</p>
+<p>A comment should be added before, on, or after 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.</p>
 <h3>Code examples</h3>
 <h4>Noncompliant code example</h4>
 <pre data-diff-id="3" data-diff-type="noncompliant">