Improve CIFuzz tests (google#4868)

1. Fix problem where permissions were being changed to root by non-root test (test was doing this by invoking test_all.py within docker).
2. Mark tests as integration tests so that cifuzz_test.py can be run in a reasonable amount of time.
3. Prevent some unittests from polluting source repo.
4. Add .venv to .gitignore
5. Rename test_test_all.py to the correctly formatted name "test_all_test.py"

Author: jonathanmetzman

.github/workflows/infra_tests.yml

@@ -33,6 +33,6 @@ jobs:
           sudo env "PATH=$PATH" gcloud info
 
       - name: Run infra tests
-        run: sudo env "PATH=$PATH" python infra/presubmit.py infra-tests
+        run: sudo env "PATH=$PATH" INTEGRATION_TESTS=1 python infra/presubmit.py infra-tests
 
 

.gitignore

@@ -4,3 +4,4 @@
 *~
 .DS_Store
 *.swp
+.venv

infra/base-images/base-runner/test_all.py

@@ -47,10 +47,16 @@ def recreate_directory(directory):
 
 def move_directory_contents(src_directory, dst_directory):
   """Moves contents of |src_directory| to |dst_directory|."""
-  src_files = os.listdir(src_directory)
-  for filename in src_files:
-    src_path = os.path.join(src_directory, filename)
-    shutil.move(src_path, dst_directory)
+  # Use mv because mv preserves file permissions. If we don't preserve file
+  # permissions that can mess up CheckFuzzerBuildTest in cifuzz_test.py and
+  # other cases where one is calling test_all on files not in OSS-Fuzz's real
+  # out directory.
+  src_contents = [
+      os.path.join(src_directory, filename)
+      for filename in os.listdir(src_directory)
+  ]
+  command = ['mv'] + src_contents + [dst_directory]
+  subprocess.check_call(command)
 
 
 def is_elf(filepath):

infra/base-images/base-runner/test_test_all.py renamed to infra/base-images/base-runner/test_all_test.py

@@ -128,6 +128,8 @@ def test_correct_host_repo_path(self, _, __):
                      os.path.basename(image_repo_path))
 
 
+@unittest.skipIf(not os.getenv('INTEGRATION_TESTS'),
+                 'INTEGRATION_TESTS=1 not set')
 class BuildFuzzersIntegrationTest(unittest.TestCase):
   """Integration tests for build_fuzzers."""
 
@@ -229,75 +231,57 @@ def test_invalid_workspace(self):
         ))
 
 
-def remove_test_files(out_parent_dir, allowlist):
-  """Removes test files from |out_parent_dir| that are not in |allowlist|, a
-  list of files with paths relative to the out directory."""
-  out_dir = os.path.join(out_parent_dir, 'out')
-  allowlist = set(allowlist)
-  for rel_out_path in os.listdir(out_dir):
-    if rel_out_path in allowlist:
-      continue
-    path_to_remove = os.path.join(out_dir, rel_out_path)
-    if os.path.isdir(path_to_remove):
-      shutil.rmtree(path_to_remove)
-    else:
-      os.remove(path_to_remove)
-
-
 class RunFuzzerIntegrationTestMixin:  # pylint: disable=too-few-public-methods,invalid-name
   """Mixin for integration test classes that runbuild_fuzzers on builds of a
   specific sanitizer."""
   # These must be defined by children.
   FUZZER_DIR = None
   FUZZER = None
 
-  def tearDown(self):
-    """Removes any existing crashes and test files."""
-    remove_test_files(self.FUZZER_DIR, self.FUZZER)
-
   def _test_run_with_sanitizer(self, fuzzer_dir, sanitizer):
     """Calls run_fuzzers on fuzzer_dir and |sanitizer| and asserts
     the run succeeded and that no bug was found."""
-    run_success, bug_found = cifuzz.run_fuzzers(10,
-                                                fuzzer_dir,
-                                                'curl',
-                                                sanitizer=sanitizer)
+    with test_helpers.temp_dir_copy(fuzzer_dir) as fuzzer_dir_copy:
+      run_success, bug_found = cifuzz.run_fuzzers(10,
+                                                  fuzzer_dir_copy,
+                                                  'curl',
+                                                  sanitizer=sanitizer)
     self.assertTrue(run_success)
     self.assertFalse(bug_found)
 
 
-class RunMemoryFuzzerIntegrationTest(unittest.TestCase,
-                                     RunFuzzerIntegrationTestMixin):
+class RunMemoryFuzzerIntegrationTest(RunFuzzerIntegrationTestMixin,
+                                     unittest.TestCase):
   """Integration test for build_fuzzers with an MSAN build."""
   FUZZER_DIR = MEMORY_FUZZER_DIR
   FUZZER = MEMORY_FUZZER
 
+  @unittest.skipIf(not os.getenv('INTEGRATION_TESTS'),
+                   'INTEGRATION_TESTS=1 not set')
   def test_run_with_memory_sanitizer(self):
     """Tests run_fuzzers with a valid MSAN build."""
     self._test_run_with_sanitizer(self.FUZZER_DIR, 'memory')
 
 
-class RunUndefinedFuzzerIntegrationTest(unittest.TestCase,
-                                        RunFuzzerIntegrationTestMixin):
+class RunUndefinedFuzzerIntegrationTest(RunFuzzerIntegrationTestMixin,
+                                        unittest.TestCase):
   """Integration test for build_fuzzers with an UBSAN build."""
   FUZZER_DIR = UNDEFINED_FUZZER_DIR
   FUZZER = UNDEFINED_FUZZER
 
+  @unittest.skipIf(not os.getenv('INTEGRATION_TESTS'),
+                   'INTEGRATION_TESTS=1 not set')
   def test_run_with_undefined_sanitizer(self):
-    """Tests run_fuzzers with a valid MSAN build."""
+    """Tests run_fuzzers with a valid UBSAN build."""
     self._test_run_with_sanitizer(self.FUZZER_DIR, 'undefined')
 
 
-class RunAddressFuzzersIntegrationTest(unittest.TestCase):
+class RunAddressFuzzersIntegrationTest(RunFuzzerIntegrationTestMixin,
+                                       unittest.TestCase):
   """Integration tests for build_fuzzers with an ASAN build."""
 
-  def tearDown(self):
-    """Removes any existing crashes and test files."""
-    files_to_keep = [
-        'undefined', 'memory', EXAMPLE_CRASH_FUZZER, EXAMPLE_NOCRASH_FUZZER
-    ]
-    remove_test_files(TEST_FILES_PATH, files_to_keep)
-
+  @unittest.skipIf(not os.getenv('INTEGRATION_TESTS'),
+                   'INTEGRATION_TESTS=1 not set')
   def test_new_bug_found(self):
     """Tests run_fuzzers with a valid ASAN build."""
     # Set the first return value to True, then the second to False to
@@ -314,6 +298,8 @@ def test_new_bug_found(self):
       self.assertTrue(run_success)
       self.assertTrue(bug_found)
 
+  @unittest.skipIf(not os.getenv('INTEGRATION_TESTS'),
+                   'INTEGRATION_TESTS=1 not set')
   def test_old_bug_found(self):
     """Tests run_fuzzers with a bug found in OSS-Fuzz before."""
     with mock.patch.object(fuzz_target.FuzzTarget,
@@ -409,6 +395,8 @@ def test_allow_broken_fuzz_targets_percentage(self, mocked_docker_run):
                   ' '.join(mocked_docker_run.call_args[0][0]))
 
 
+@unittest.skipIf(not os.getenv('INTEGRATION_TESTS'),
+                 'INTEGRATION_TESTS=1 not set')
 class GetFilesCoveredByTargetTest(unittest.TestCase):
   """Tests get_files_covered_by_target."""
 

infra/cifuzz/cifuzz_test.py

@@ -13,7 +13,10 @@
 # limitations under the License.
 """Contains convenient helpers for writing tests."""
 
+import contextlib
 import os
+import shutil
+import tempfile
 from unittest import mock
 
 
@@ -25,3 +28,12 @@ def patch_environ(testcase_obj, env=None):
   patcher = mock.patch.dict(os.environ, env)
   testcase_obj.addCleanup(patcher.stop)
   patcher.start()
+
+
+@contextlib.contextmanager
+def temp_dir_copy(directory):
+  """Context manager that yields a temporary copy of |directory|."""
+  with tempfile.TemporaryDirectory() as temp_dir:
+    temp_copy_path = os.path.join(temp_dir, os.path.basename(directory))
+    shutil.copytree(directory, temp_copy_path)
+    yield temp_copy_path