From 86f79e97cd7838e53bde10776f876c64b7808816 Mon Sep 17 00:00:00 2001
From: Yassin Kammoun
 <52890329+yassin-kammoun-sonarsource@users.noreply.github.com>
Date: Fri, 6 Sep 2024 17:01:12 +0200
Subject: [PATCH] JS-312 Improve S1607 (`no-skipped-tests`): Allow explanation
 comments adjacent to the disabled test (#4804)

---
 .../S1607/fixtures/jasmine/cb.fixture.js      | 30 +++++++++++++++----
 .../rules/S1607/fixtures/jest/cb.fixture.js   | 25 +++++++++++-----
 .../rules/S1607/fixtures/mocha/cb.fixture.js  | 13 +++++---
 packages/jsts/src/rules/S1607/rule.ts         | 21 ++++++++-----
 .../javascript/rules/javascript/S1607.html    | 12 ++++----
 5 files changed, 72 insertions(+), 29 deletions(-)

diff --git a/packages/jsts/src/rules/S1607/fixtures/jasmine/cb.fixture.js b/packages/jsts/src/rules/S1607/fixtures/jasmine/cb.fixture.js
index 01be4413116..bc92184a96e 100644
--- a/packages/jsts/src/rules/S1607/fixtures/jasmine/cb.fixture.js
+++ b/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();
   });
 });
diff --git a/packages/jsts/src/rules/S1607/fixtures/jest/cb.fixture.js b/packages/jsts/src/rules/S1607/fixtures/jest/cb.fixture.js
index 0ad675299f6..2dd10a01d57 100644
--- a/packages/jsts/src/rules/S1607/fixtures/jest/cb.fixture.js
+++ b/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();
   });
diff --git a/packages/jsts/src/rules/S1607/fixtures/mocha/cb.fixture.js b/packages/jsts/src/rules/S1607/fixtures/mocha/cb.fixture.js
index 87b12267be3..f261b9794c4 100644
--- a/packages/jsts/src/rules/S1607/fixtures/mocha/cb.fixture.js
+++ b/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();
   });
diff --git a/packages/jsts/src/rules/S1607/rule.ts b/packages/jsts/src/rules/S1607/rule.ts
index b5125c22cc0..72930415ee5 100644
--- a/packages/jsts/src/rules/S1607/rule.ts
+++ b/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));
     }
   },
 };
diff --git a/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1607.html b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1607.html
index 625319d159d..30ca7a8851e 100644
--- a/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1607.html
+++ b/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">