staticanalysis/bot: Use actual number of lines in the file to identif…

…y full file issues (#1881)

src/staticanalysis/bot/static_analysis_bot/coverage.py

@@ -30,18 +30,22 @@ class CoverageIssue(Issue):
     ANALYZER = COVERAGE
 
     def __init__(self, path, lineno, message, revision):
-        self.nb_lines = -1
-        self.line = lineno and int(lineno) or 0
-        self.message = message
-        self.revision = revision
-
         # Ensure path is always relative to the repository
         self.path = path
         if self.path.startswith(settings.repo_dir):
             self.path = os.path.relpath(self.path, settings.repo_dir)
-        assert os.path.exists(os.path.join(settings.repo_dir, self.path)), \
+
+        full_path = os.path.join(settings.repo_dir, self.path)
+
+        assert os.path.exists(full_path), \
             'Missing {} in repo {}'.format(self.path, settings.repo_dir)
 
+        self.line = lineno and int(lineno) or 0
+        with open(full_path) as f:
+            self.nb_lines = sum(1 for line in f)
+        self.message = message
+        self.revision = revision
+
     def __str__(self):
         return self.path
 

src/staticanalysis/bot/static_analysis_bot/revisions.py

@@ -113,10 +113,6 @@ def contains(self, issue):
             logger.warn('Issue path in not in revision', path=issue.path, revision=self)
             return False
 
-        # If nb_lines is -1, it means the issue applies to the entire file.
-        if issue.nb_lines == -1:
-            return True
-
         # Detect if this issue is in the patch
         lines = set(range(issue.line, issue.line + issue.nb_lines))
         return not lines.isdisjoint(modified_lines)

src/staticanalysis/bot/tests/test_coverage.py

@@ -21,14 +21,14 @@ def test_coverage(mock_config, mock_repository, mock_revision, mock_coverage):
 
     # Build fake lines.
     for path in mock_revision.files:
-        mock_revision.lines[path] = [1]
+        mock_revision.lines[path] = [0]
 
     # Build fake files.
-    for path in mock_revision.files:
+    for i, path in enumerate(mock_revision.files):
         full_path = os.path.join(mock_config.repo_dir, path)
         os.makedirs(os.path.dirname(full_path), exist_ok=True)
         with open(full_path, 'w') as f:
-            f.write('line')
+            f.write('line\n' * (i + 1))
 
     issues = cov.run(mock_revision)
 
@@ -42,14 +42,16 @@ def test_coverage(mock_config, mock_repository, mock_revision, mock_coverage):
     assert issue.message == 'This file is uncovered'
     assert str(issue) == 'my/path/file1.cpp'
 
+    assert issue.build_lines_hash() == 'c73b73af8851e9e91bc6b4dc12e7dace0a2bfb931c1d0b8b36ef367319f58cd1'
+
     assert not issue.is_third_party()
     assert issue.validates()
 
     assert issue.as_dict() == {
         'analyzer': 'coverage',
         'path': 'my/path/file1.cpp',
         'line': 0,
-        'nb_lines': -1,
+        'nb_lines': 1,
         'message': 'This file is uncovered',
         'is_third_party': False,
         'in_patch': True,
@@ -77,14 +79,16 @@ def test_coverage(mock_config, mock_repository, mock_revision, mock_coverage):
     assert issue.message == 'This file is uncovered'
     assert str(issue) == 'test/dummy/thirdparty.c'
 
+    assert issue.build_lines_hash() == '0b3ed69ca643edb6acc6fb43a1b9a235d7e773d43818396d4e05ead684e8f7c9'
+
     assert issue.is_third_party()
     assert issue.validates()
 
     assert issue.as_dict() == {
         'analyzer': 'coverage',
         'path': 'test/dummy/thirdparty.c',
         'line': 0,
-        'nb_lines': -1,
+        'nb_lines': 3,
         'message': 'This file is uncovered',
         'is_third_party': True,
         'in_patch': True,

src/staticanalysis/bot/tests/test_reporter_phabricator.py

@@ -196,7 +196,7 @@ def _check_comment(request):
         revision = PhabricatorRevision('PHID-DIFF-abcdef', api)
         revision.lines = {
             # Add dummy lines diff
-            'test.txt': [41, 42, 43],
+            'test.txt': [0],
             'dom/test.cpp': [42, ],
         }
         reporter = PhabricatorReporter({'analyzers': ['coverage']}, api=api)
@@ -276,7 +276,7 @@ def _check_comment_ccov(request):
         revision = PhabricatorRevision('PHID-DIFF-abcdef', api)
         revision.lines = {
             # Add dummy lines diff
-            'test.txt': [41, 42, 43],
+            'test.txt': [0],
             'test.cpp': [41, 42, 43],
         }
         reporter = PhabricatorReporter({'analyzers': ['coverage', 'clang-tidy']}, api=api)
@@ -316,6 +316,7 @@ def test_phabricator_analyzers(mock_config, mock_repository, mock_phabricator):
     from static_analysis_bot.infer.infer import InferIssue
     from static_analysis_bot.clang.tidy import ClangTidyIssue
     from static_analysis_bot.lint import MozLintIssue
+    from static_analysis_bot.coverage import CoverageIssue
 
     # needed by Mozlint issue
     with open(os.path.join(mock_config.repo_dir, 'test.cpp'), 'w') as f:
@@ -325,7 +326,7 @@ def _test_reporter(api, analyzers):
         # Always use the same setup, only varies the analyzers
         revision = PhabricatorRevision('PHID-DIFF-abcdef', api)
         revision.lines = {
-            'test.cpp': [41, 42, 43],
+            'test.cpp': [0, 41, 42, 43],
             'dom/test.cpp': [42, ],
         }
         reporter = PhabricatorReporter({'analyzers': analyzers}, api=api)
@@ -342,6 +343,7 @@ def _test_reporter(api, analyzers):
                 'qualifier': 'dummy message.',
             }, revision),
             MozLintIssue('test.cpp', 1, 'danger', 42, 'flake8', 'Python error', 'EXXX', revision),
+            CoverageIssue('test.cpp', 0, 'This file is uncovered', revision),
         ]
 
         assert all(i.is_publishable() for i in issues)
@@ -392,14 +394,19 @@ def _test_reporter(api, analyzers):
         assert len(patches) == 1
         assert [p.analyzer for p in patches] == ['mozlint']
 
+        # Only coverage
+        issues, patches = _test_reporter(api, ['coverage'])
+        assert len(issues) == 1
+        assert len(patches) == 0
+
         # clang-format + clang-tidy
         issues, patches = _test_reporter(api, ['clang-tidy', 'clang-format'])
         assert len(issues) == 2
         assert len(patches) == 2
         assert [p.analyzer for p in patches] == ['clang-tidy', 'clang-format']
 
         # All of them
-        issues, patches = _test_reporter(api, ['clang-tidy', 'clang-format', 'infer', 'mozlint'])
-        assert len(issues) == 4
+        issues, patches = _test_reporter(api, ['clang-tidy', 'clang-format', 'infer', 'mozlint', 'coverage'])
+        assert len(issues) == 5
         assert len(patches) == 4
         assert [p.analyzer for p in patches] == ['clang-tidy', 'clang-format', 'infer', 'mozlint']