Ignore enhancements (#454)

* typo and case fixes * allow omitting space between // and coverage * allow text after coverage:ignore comments * handle unbalanced ignore ranges instead of bailing * add changelog entry * update title in changelog * throw FormatException when encountering unbalanced ignore comments * add file path to err message and test err message * refactor test
dart-archive · Jul 6, 2023 · 19f7189 · 19f7189
1 parent 3dcefbc
commit 19f7189
Show file tree

Hide file tree

Showing 4 changed files with 126 additions and 23 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,9 @@
+## 1.6.4-wip.
+
+- allow omitting space between `//` and `coverage` in coverage ignore comments
+- allow text after coverage ignore comments
+- throw FormatException when encountering unbalanced ignore comments instead of silently erroring
+
 ## 1.6.3
 
 - Require Dart 2.18

diff --git a/lib/src/hitmap.dart b/lib/src/hitmap.dart
@@ -76,7 +76,7 @@ class HitMap {
           final path = resolver.resolve(source);
           if (path != null) {
             final lines = loader.loadSync(path) ?? [];
-            ignoredLinesList = getIgnoredLines(lines);
+            ignoredLinesList = getIgnoredLines(path, lines);
 
             // Ignore the whole file.
             if (ignoredLinesList.length == 1 &&

diff --git a/lib/src/util.dart b/lib/src/util.dart
@@ -74,13 +74,13 @@ Future<int> getOpenPort() async {
   }
 }
 
-final muliLineIgnoreStart = RegExp(r'// coverage:ignore-start\s*$');
-final muliLineIgnoreEnd = RegExp(r'// coverage:ignore-end\s*$');
-final singleLineIgnore = RegExp(r'// coverage:ignore-line\s*$');
-final ignoreFile = RegExp(r'// coverage:ignore-file\s*$');
+final muliLineIgnoreStart = RegExp(r'//\s*coverage:ignore-start[\w\d\s]*$');
+final muliLineIgnoreEnd = RegExp(r'//\s*coverage:ignore-end[\w\d\s]*$');
+final singleLineIgnore = RegExp(r'//\s*coverage:ignore-line[\w\d\s]*$');
+final ignoreFile = RegExp(r'//\s*coverage:ignore-file[\w\d\s]*$');
 
 /// Return list containing inclusive range of lines to be ignored by coverage.
-/// If there is a error in balancing the statements it will ignore nothing,
+/// If there is a error in balancing the statements it will throw a FormatException,
 /// unless `coverage:ignore-file` is found.
 /// Return [0, lines.length] if the whole file is ignored.
 ///
@@ -100,44 +100,64 @@ final ignoreFile = RegExp(r'// coverage:ignore-file\s*$');
 /// ]
 /// ```
 ///
-List<List<int>> getIgnoredLines(List<String>? lines) {
+List<List<int>> getIgnoredLines(String filePath, List<String>? lines) {
   final ignoredLines = <List<int>>[];
   if (lines == null) return ignoredLines;
 
   final allLines = [
     [0, lines.length]
   ];
 
-  var isError = false;
+  FormatException? err;
   var i = 0;
   while (i < lines.length) {
     if (lines[i].contains(ignoreFile)) return allLines;
 
-    if (lines[i].contains(muliLineIgnoreEnd)) isError = true;
+    if (lines[i].contains(muliLineIgnoreEnd)) {
+      err ??= FormatException(
+        'unmatched coverage:ignore-end found at $filePath:${i + 1}',
+      );
+    }
 
     if (lines[i].contains(singleLineIgnore)) ignoredLines.add([i + 1, i + 1]);
 
     if (lines[i].contains(muliLineIgnoreStart)) {
       final start = i;
+      var isUnmatched = true;
       ++i;
       while (i < lines.length) {
         if (lines[i].contains(ignoreFile)) return allLines;
         if (lines[i].contains(muliLineIgnoreStart)) {
-          isError = true;
+          err ??= FormatException(
+            'coverage:ignore-start found at $filePath:${i + 1}'
+            ' before previous coverage:ignore-start ended',
+          );
           break;
         }
 
         if (lines[i].contains(muliLineIgnoreEnd)) {
           ignoredLines.add([start + 1, i + 1]);
+          isUnmatched = false;
           break;
         }
         ++i;
       }
+
+      if (isUnmatched) {
+        err ??= FormatException(
+          'coverage:ignore-start found at $filePath:${start + 1}'
+          ' has no matching coverage:ignore-end',
+        );
+      }
     }
     ++i;
   }
 
-  return isError ? [] : ignoredLines;
+  if (err == null) {
+    return ignoredLines;
+  }
+
+  throw err;
 }
 
 extension StandardOutExtension on Stream<List<int>> {

diff --git a/test/util_test.dart b/test/util_test.dart
@@ -176,10 +176,53 @@ void main() {
         ''',
     ];
 
-    test('returns empty when the annotations are not balanced', () {
-      for (final content in invalidSources) {
-        expect(getIgnoredLines(content.split('\n')), isEmpty);
+    test('throws FormatException when the annotations are not balanced', () {
+      void runTest(int index, String errMsg) {
+        final content = invalidSources[index].split('\n');
+        expect(
+          () => getIgnoredLines('content-$index.dart', content),
+          throwsA(
+            allOf(
+              isFormatException,
+              predicate((FormatException e) => e.message == errMsg),
+            ),
+          ),
+          reason: 'expected FormatException with message "$errMsg"',
+        );
       }
+
+      runTest(
+        0,
+        'coverage:ignore-start found at content-0.dart:3 before previous coverage:ignore-start ended',
+      );
+      runTest(
+        1,
+        'coverage:ignore-start found at content-1.dart:3 before previous coverage:ignore-start ended',
+      );
+      runTest(
+        2,
+        'unmatched coverage:ignore-end found at content-2.dart:5',
+      );
+      runTest(
+        3,
+        'unmatched coverage:ignore-end found at content-3.dart:1',
+      );
+      runTest(
+        4,
+        'unmatched coverage:ignore-end found at content-4.dart:1',
+      );
+      runTest(
+        5,
+        'unmatched coverage:ignore-end found at content-5.dart:1',
+      );
+      runTest(
+        6,
+        'unmatched coverage:ignore-end found at content-6.dart:1',
+      );
+      runTest(
+        7,
+        'coverage:ignore-start found at content-7.dart:1 has no matching coverage:ignore-end',
+      );
     });
 
     test(
@@ -188,20 +231,20 @@ void main() {
       for (final content in invalidSources) {
         final lines = content.split('\n');
         lines.add(' // coverage:ignore-file');
-        expect(getIgnoredLines(lines), [
+        expect(getIgnoredLines('', lines), [
           [0, lines.length]
         ]);
       }
     });
 
-    test('Returns [[0,lines.length]] when the whole file is ignored', () {
+    test('returns [[0,lines.length]] when the whole file is ignored', () {
       final lines = '''final str = ''; // coverage:ignore-start
       final str = ''; // coverage:ignore-end
       final str = ''; // coverage:ignore-file
       '''
           .split('\n');
 
-      expect(getIgnoredLines(lines), [
+      expect(getIgnoredLines('', lines), [
         [0, lines.length]
       ]);
     });
@@ -217,7 +260,7 @@ void main() {
       '''
           .split('\n');
 
-      expect(getIgnoredLines(lines), [
+      expect(getIgnoredLines('', lines), [
         [1, 3],
         [4, 6],
       ]);
@@ -231,14 +274,14 @@ void main() {
       '''
           .split('\n');
 
-      expect(getIgnoredLines(lines), [
+      expect(getIgnoredLines('', lines), [
         [1, 1],
         [2, 2],
         [3, 3],
       ]);
     });
 
-    test('Ingore comments have no effect inside string literals', () {
+    test('ignore comments have no effect inside string literals', () {
       final lines = '''
       final str = '// coverage:ignore-file';
       final str = '// coverage:ignore-line';
@@ -248,12 +291,12 @@ void main() {
       '''
           .split('\n');
 
-      expect(getIgnoredLines(lines), [
+      expect(getIgnoredLines('', lines), [
         [3, 3],
       ]);
     });
 
-    test('Allow white-space after ignore comments', () {
+    test('allow white-space after ignore comments', () {
       // Using multiple strings, rather than splitting a multi-line string,
       // because many code editors remove trailing white-space.
       final lines = [
@@ -265,7 +308,41 @@ void main() {
         "final str = ''; // coverage:ignore-end  \t    \t ",
       ];
 
-      expect(getIgnoredLines(lines), [
+      expect(getIgnoredLines('', lines), [
+        [1, 3],
+        [4, 4],
+        [5, 6],
+      ]);
+    });
+
+    test('allow omitting space after //', () {
+      final lines = [
+        "final str = ''; //coverage:ignore-start",
+        "final str = ''; //coverage:ignore-line",
+        "final str = ''; //coverage:ignore-end",
+        "final str = ''; //coverage:ignore-line",
+        "final str = ''; //coverage:ignore-start",
+        "final str = ''; //coverage:ignore-end",
+      ];
+
+      expect(getIgnoredLines('', lines), [
+        [1, 3],
+        [4, 4],
+        [5, 6],
+      ]);
+    });
+
+    test('allow text after ignore comments', () {
+      final lines = [
+        "final str = ''; // coverage:ignore-start due to XYZ",
+        "final str = ''; // coverage:ignore-line",
+        "final str = ''; // coverage:ignore-end due to XYZ",
+        "final str = ''; // coverage:ignore-line due to 123",
+        "final str = ''; // coverage:ignore-start",
+        "final str = ''; // coverage:ignore-end it is done",
+      ];
+
+      expect(getIgnoredLines('', lines), [
         [1, 3],
         [4, 4],
         [5, 6],