Fix unittests and make sure functionality stays same

3 files changed, 52 insertions, 45 deletions

diff --git a/infra/cifuzz/affected_fuzz_targets.py b/infra/cifuzz/affected_fuzz_targets.py
index e8c42a79e..fb09eb644 100644
--- a/infra/cifuzz/affected_fuzz_targets.py
+++ b/infra/cifuzz/affected_fuzz_targets.py
@@ -54,7 +54,7 @@ def remove_unaffected_fuzz_targets(project_name, out_dir, files_changed,
   coverage_getter = coverage.OssFuzzCoverageGetter(project_name, repo_path)
   if not coverage_getter.fuzzer_stats_url:
     # Don't remove any fuzz targets unless we have data.
-    logging.error('Could not download latest coverage report.')
+    logging.error('Could not find latest coverage report.')
     return
 
   affected_fuzz_targets = get_affected_fuzz_targets(coverage_getter,
@@ -86,6 +86,8 @@ def is_fuzz_target_affected(coverage_getter, fuzz_target_path, files_changed):
   if not covered_files:
     # Assume a fuzz target is affected if we can't get its coverage from
     # OSS-Fuzz.
+    # TODO(metzman): Figure out what we should do if covered_files is [].
+    # Should we act as if we couldn't get the coverage?
     logging.info('Could not get coverage for %s. Treating as affected.',
                  fuzz_target)
     return True
diff --git a/infra/cifuzz/coverage.py b/infra/cifuzz/coverage.py
index 6cfea34b2..b5c6fbf1a 100644
--- a/infra/cifuzz/coverage.py
+++ b/infra/cifuzz/coverage.py
@@ -31,6 +31,8 @@ class OssFuzzCoverageGetter:
   """Gets coverage data for a project from OSS-Fuzz."""
 
   def __init__(self, project_name, repo_path):
+    """Constructor for OssFuzzCoverageGetter. Callers should check that
+    fuzzer_stats_url is initialized."""
     self.project_name = project_name
     self.repo_path = _normalize_repo_path(repo_path)
     self.fuzzer_stats_url = _get_fuzzer_stats_dir_url(self.project_name)
@@ -39,12 +41,14 @@ class OssFuzzCoverageGetter:
     """Get the coverage report for a specific fuzz target.
 
     Args:
-      latest_cov_info: A dict containing a project's latest cov report info.
-      target_name: The name of the fuzz target whose coverage is requested.
+      target: The name of the fuzz target whose coverage is requested.
 
     Returns:
-      The targets coverage json dict or None on failure.
+      The target's coverage json dict or None on failure.
     """
+    if not self.fuzzer_stats_url:
+      return None
+
     target_url = utils.url_join(self.fuzzer_stats_url, target + '.json')
     return get_json_from_url(target_url)
 
@@ -52,16 +56,13 @@ class OssFuzzCoverageGetter:
     """Gets a list of source files covered by the specific fuzz target.
 
     Args:
-      latest_cov_info: A dict containing a project's latest cov report info.
-      target_name: The name of the fuzz target whose coverage is requested.
-      oss_fuzz_repo_path: The location of the repo in the docker image.
+      target: The name of the fuzz target whose coverage is requested.
 
     Returns:
       A list of files that the fuzz targets covers or None.
     """
     target_cov = self.get_target_coverage_report(target)
     if not target_cov:
-      logging.error('No coverage data for %s', target)
       return None
 
     coverage_per_file = get_coverage_per_file(target_cov)
@@ -73,12 +74,15 @@ class OssFuzzCoverageGetter:
     for file_cov in coverage_per_file:
       norm_file_path = os.path.normpath(file_cov['filename'])
       if not norm_file_path.startswith(self.repo_path):
+        # Exclude files outside of the main repo.
         continue
 
       if not is_file_covered(file_cov):
         # Don't consider a file affected if code in it is never executed.
         continue
 
+      # TODO(metzman): It's weird to me that we access file_cov['filename']
+      # again and not norm_file_path, figure out if this makes sense.
       relative_path = utils.remove_prefix(file_cov['filename'], self.repo_path)
       affected_file_list.append(relative_path)
 
@@ -111,7 +115,7 @@ def _get_latest_cov_report_info(project_name):
                                           LATEST_REPORT_INFO_PATH,
                                           project_name + '.json')
   latest_cov_info = get_json_from_url(latest_report_info_url)
-  if latest_cov_info is None:
+  if not latest_cov_info is None:
     logging.error('Could not get the coverage report json from url: %s.',
                   latest_report_info_url)
     return None
@@ -128,9 +132,14 @@ def _get_fuzzer_stats_dir_url(project_name):
     The projects coverage report info in json dict or None on failure.
   """
   latest_cov_info = _get_latest_cov_report_info(project_name)
-  # !!! Make sure functionally the same as before.
-  if latest_cov_info is None or 'fuzzer_stats_dir' not in latest_cov_info:
+
+  if not latest_cov_info:
     return None
+
+  if 'fuzzer_stats_dir' not in latest_cov_info:
+    logging.error('fuzzer_stats_dir not in latest coverage info.')
+    return None
+
   fuzzer_stats_dir_gs_url = latest_cov_info['fuzzer_stats_dir']
   fuzzer_stats_dir_url = utils.gs_url_to_https(fuzzer_stats_dir_gs_url)
   return fuzzer_stats_dir_url
diff --git a/infra/cifuzz/coverage_test.py b/infra/cifuzz/coverage_test.py
index 3db608aba..d94433a4d 100644
--- a/infra/cifuzz/coverage_test.py
+++ b/infra/cifuzz/coverage_test.py
@@ -24,20 +24,28 @@ import coverage
 TEST_FILES_PATH = os.path.join(os.path.dirname(os.path.abspath(__file__)),
                                'test_files')
 
+PROJECT_NAME = 'curl'
+REPO_PATH = '/src/curl'
+FUZZER = 'curl_fuzzer'
+PROJECT_COV_JSON_FILENAME = 'example_curl_cov.json'
+FUZZER_COV_JSON_FILENAME = 'example_curl_fuzzer_cov.json'
+
+with open(os.path.join(TEST_FILES_PATH,
+                       PROJECT_COV_JSON_FILENAME),) as cov_file_handle:
+  PROJECT_COV_INFO = json.loads(cov_file_handle.read())
+
 
 class GetFuzzerStatsDirUrlTest(unittest.TestCase):
   """Tests _get_fuzzer_stats_dir_url."""
 
-  TEST_PROJECT = 'curl'
-
   @mock.patch('coverage.get_json_from_url', return_value={})
   def test_get_valid_project(self, mocked_get_json_from_url):
     """Tests that a project's coverage report can be downloaded and parsed.
 
-    NOTE: This test relies on the TEST_PROJECT repo's coverage report.
+    NOTE: This test relies on the PROJECT_NAME repo's coverage report.
     The "example" project was not used because it has no coverage reports.
     """
-    coverage._get_fuzzer_stats_dir_url(self.TEST_PROJECT)
+    coverage._get_fuzzer_stats_dir_url(PROJECT_NAME)
     (url,), _ = mocked_get_json_from_url.call_args
     self.assertEqual(
         'https://storage.googleapis.com/oss-fuzz-coverage/'
@@ -51,24 +59,16 @@ class GetFuzzerStatsDirUrlTest(unittest.TestCase):
 class GetTargetCoverageReportTest(unittest.TestCase):
   """Tests get_target_coverage_report."""
 
-  EXAMPLE_COV_JSON = 'example_curl_cov.json'
-  EXAMPLE_FUZZER = 'curl_fuzzer'
-
   def setUp(self):
-    with open(os.path.join(TEST_FILES_PATH, self.EXAMPLE_COV_JSON),
-              'r') as file_handle:
-      example_cov_info = json.loads(file_handle.read())
-    project_name = 'curl'
-    repo_path = '/src/curl'
     with mock.patch('coverage._get_latest_cov_report_info',
-                    return_value=example_cov_info):
+                    return_value=PROJECT_COV_INFO):
       self.coverage_getter = coverage.OssFuzzCoverageGetter(
-          project_name, repo_path)
+          PROJECT_NAME, REPO_PATH)
 
   @mock.patch('coverage.get_json_from_url', return_value={})
   def test_valid_target(self, mocked_get_json_from_url):
     """Tests that a target's coverage report can be downloaded and parsed."""
-    self.coverage_getter.get_target_coverage_report(self.EXAMPLE_FUZZER)
+    self.coverage_getter.get_target_coverage_report(FUZZER)
     (url,), _ = mocked_get_json_from_url.call_args
     self.assertEqual(
         'https://storage.googleapis.com/oss-fuzz-coverage/'
@@ -83,36 +83,32 @@ class GetTargetCoverageReportTest(unittest.TestCase):
 class GetFilesCoveredByTargetTest(unittest.TestCase):
   """Tests get_files_covered_by_target."""
 
-  FUZZER_COV_JSON_FILENAME = 'example_curl_fuzzer_cov.json'
-  EXAMPLE_FUZZER = 'curl_fuzzer'
-  PROJECT_NAME = 'curl'
-  REPO_PATH = '/src/curl'
-
   def setUp(self):
-    example_cov_json = 'example_curl_cov.json'
-    with open(os.path.join(TEST_FILES_PATH, example_cov_json)) as file_handle:
-      self.cov_report = json.loads(file_handle.read())
-
     with mock.patch('coverage._get_latest_cov_report_info',
-                    return_value=self.cov_report):
+                    return_value=PROJECT_COV_INFO):
       self.coverage_getter = coverage.OssFuzzCoverageGetter(
-          self.PROJECT_NAME, self.REPO_PATH)
-    with open(os.path.join(TEST_FILES_PATH,
-                           self.FUZZER_COV_JSON_FILENAME)) as file_handle:
-      self.fuzzer_cov_report = json.loads(file_handle.read())
+          PROJECT_NAME, REPO_PATH)
 
   def test_valid_target(self):
     """Tests that covered files can be retrieved from a coverage report."""
+    with open(os.path.join(TEST_FILES_PATH,
+                           FUZZER_COV_JSON_FILENAME),) as file_handle:
+      fuzzer_cov_info = json.loads(file_handle.read())
+
     with mock.patch('coverage.OssFuzzCoverageGetter.get_target_coverage_report',
-                    return_value=self.fuzzer_cov_report):
-      file_list = self.coverage_getter.get_files_covered_by_target(
-          self.EXAMPLE_FUZZER)
+                    return_value=fuzzer_cov_info):
+      file_list = self.coverage_getter.get_files_covered_by_target(FUZZER)
 
     curl_files_list_path = os.path.join(TEST_FILES_PATH,
                                         'example_curl_file_list.json')
     with open(curl_files_list_path) as file_handle:
-      true_files_list = json.load(file_handle)
-    self.assertCountEqual(file_list, true_files_list)
+      expected_file_list = json.load(file_handle)
+    self.assertCountEqual(file_list, expected_file_list)
+
+  def test_invalid_target(self):
+    """Tests passing invalid fuzz target returns None."""
+    self.assertIsNone(
+        self.coverage_getter.get_files_covered_by_target('not-a-fuzzer'))
 
 
 class IsFileCoveredTest(unittest.TestCase):