diff options
| -rw-r--r-- | .travis.yml | 4 | ||||
| -rw-r--r-- | infra/cifuzz/fuzz_target.py | 108 | ||||
| -rw-r--r-- | infra/cifuzz/fuzz_target_test.py | 270 | ||||
| -rw-r--r-- | infra/dev-requirements.txt | 5 | ||||
| -rwxr-xr-x | infra/presubmit.py | 12 | ||||
| -rw-r--r-- | infra/travis/requirements.txt | 5 |
6 files changed, 262 insertions, 142 deletions
diff --git a/.travis.yml b/.travis.yml index 879dbb63..3cf7704a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -14,8 +14,6 @@ install: matrix: include: - name: "presubmit" - install: - - pip install -r infra/dev-requirements.txt script: ./infra/presubmit.py - name: "libfuzzer address x86_64" env: @@ -58,7 +56,7 @@ matrix: - TRAVIS_SANITIZER=dataflow - TRAVIS_ARCHITECTURE=x86_64 - name: "infra-tests" - script: sudo ./infra/presubmit.py infra-tests + script: sudo /bin/bash -c 'source $HOME/virtualenv/python3.6/bin/activate && ./infra/presubmit.py infra-tests' script: ./infra/travis/travis_build.py diff --git a/infra/cifuzz/fuzz_target.py b/infra/cifuzz/fuzz_target.py index 85fdbdc9..d32322a9 100644 --- a/infra/cifuzz/fuzz_target.py +++ b/infra/cifuzz/fuzz_target.py @@ -57,9 +57,20 @@ SANITIZER = 'address' # The number of reproduce attempts for a crash. REPRODUCE_ATTEMPTS = 10 -# Seconds on top of duration till a timeout error is raised. +# Seconds on top of duration until a timeout error is raised. BUFFER_TIME = 10 +# Log message for is_crash_reportable if it can't check if crash repros +# on OSS-Fuzz build. +COULD_NOT_TEST_ON_OSS_FUZZ_MESSAGE = ( + 'Crash is reproducible. Could not run OSS-Fuzz build of ' + 'target to determine if this pull request introduced crash. ' + 'Assuming this pull request introduced crash.') + + +class ReproduceError(Exception): + """Error for when we can't attempt to reproduce a crash.""" + class FuzzTarget: """A class to manage a single fuzz target. @@ -134,20 +145,20 @@ class FuzzTarget: logging.error('Fuzzer %s timed out, ending fuzzing.', self.target_name) return None, None - # Libfuzzer timeout has been reached. + # Libfuzzer timeout was reached. if not process.returncode: logging.info('Fuzzer %s finished with no crashes discovered.', self.target_name) return None, None - # Crash has been discovered. + # Crash was discovered. logging.info('Fuzzer %s, ended before timeout.', self.target_name) err_str = err.decode('ascii') test_case = self.get_test_case(err_str) if not test_case: logging.error('No test case found in stack trace: %s.', err_str) return None, None - if self.check_reproducibility_and_regression(test_case): + if self.is_crash_reportable(test_case): return test_case, err_str return None, None @@ -159,24 +170,30 @@ class FuzzTarget: target_path: The path to the fuzz target to be tested Returns: - True if crash is reproducible. + True if crash is reproducible and we were able to run the + binary. + + Raises: + ReproduceError if we can't attempt to reproduce the crash. """ - if not os.path.exists(test_case): - logging.error('Test case %s is not found.', test_case) - return False - if os.path.exists(target_path): - os.chmod(os.path.join(target_path, self.target_name), stat.S_IRWXO) + if not os.path.exists(target_path): + raise ReproduceError('Target %s not found.' % target_path) + + os.chmod(target_path, stat.S_IRWXO) + + target_dirname = os.path.dirname(target_path) command = ['docker', 'run', '--rm', '--privileged'] container = utils.get_container_name() if container: command += [ - '--volumes-from', container, '-e', 'OUT=' + target_path, '-e', + '--volumes-from', container, '-e', 'OUT=' + target_dirname, '-e', 'TESTCASE=' + test_case ] else: command += [ - '-v', '%s:/out' % target_path, '-v', + '-v', + '%s:/out' % target_dirname, '-v', '%s:/testcase' % test_case ] @@ -187,14 +204,21 @@ class FuzzTarget: logging.info('Running reproduce command: %s.', ' '.join(command)) for _ in range(REPRODUCE_ATTEMPTS): - _, _, err_code = utils.execute(command) - if err_code: + _, _, returncode = utils.execute(command) + if returncode != 0: + logging.info('Reproduce command returned: %s. Reproducible on %s.', + returncode, target_path) + return True + + logging.info('Reproduce command returned 0. Not reproducible on %s.', + target_path) return False - def check_reproducibility_and_regression(self, test_case): - """Checks if a crash is reproducible, and if it is, whether it's a new - regression that cannot be reproduced with the latest OSS-Fuzz build. + def is_crash_reportable(self, test_case): + """Returns True if a crash is reportable. This means the crash is + reproducible but not reproducible on a build from OSS-Fuzz (meaning the + crash was introduced by this PR). NOTE: If no project is specified the crash is assumed introduced by the pull request if it is reproducible. @@ -204,28 +228,52 @@ class FuzzTarget: Returns: True if the crash was introduced by the current pull request. + + Raises: + ReproduceError if we can't attempt to reproduce the crash on the PR build. """ - reproducible_in_pr = self.is_reproducible(test_case, - os.path.dirname(self.target_path)) + if not os.path.exists(test_case): + raise ReproduceError('Test case %s not found.' % test_case) + + try: + reproducible_on_pr_build = self.is_reproducible(test_case, + self.target_path) + except ReproduceError as error: + logging.error('Could not run target when checking for reproducibility.' + 'Please file an issue:' + 'https://github.com/google/oss-fuzz/issues/new.') + raise error + if not self.project_name: - return reproducible_in_pr + return reproducible_on_pr_build - if not reproducible_in_pr: + if not reproducible_on_pr_build: logging.info( 'Failed to reproduce the crash using the obtained test case.') return False oss_fuzz_build_dir = self.download_oss_fuzz_build() if not oss_fuzz_build_dir: - return False + # Crash is reproducible on PR build and we can't test on OSS-Fuzz build. + logging.info(COULD_NOT_TEST_ON_OSS_FUZZ_MESSAGE) + return True - reproducible_in_oss_fuzz = self.is_reproducible(test_case, - oss_fuzz_build_dir) + oss_fuzz_target_path = os.path.join(oss_fuzz_build_dir, self.target_name) + try: + reproducible_on_oss_fuzz_build = self.is_reproducible( + test_case, oss_fuzz_target_path) + except ReproduceError: + # This happens if the project has OSS-Fuzz builds, but the fuzz target + # is not in it (e.g. because the fuzz target is new). + logging.info(COULD_NOT_TEST_ON_OSS_FUZZ_MESSAGE) + return True - if reproducible_in_pr and not reproducible_in_oss_fuzz: - logging.info('The crash is reproducible. The crash doesn\'t reproduce ' \ - 'on old builds. This pull request probably introduced the crash.') + if not reproducible_on_oss_fuzz_build: + logging.info('The crash is reproducible. The crash doesn\'t reproduce ' + 'on old builds. This pull request probably introduced the ' + 'crash.') return True + logging.info('The crash is reproducible without the current pull request.') return False @@ -347,13 +395,13 @@ def download_and_unpack_zip(http_url, out_dir): return out_dir -def url_join(*argv): +def url_join(*url_parts): """Joins URLs together using the posix join method. Args: - argv: Sections of a URL to be joined. + url_parts: Sections of a URL to be joined. Returns: Joined URL. """ - return posixpath.join(*argv) + return posixpath.join(*url_parts) diff --git a/infra/cifuzz/fuzz_target_test.py b/infra/cifuzz/fuzz_target_test.py index 68052746..04aac4c9 100644 --- a/infra/cifuzz/fuzz_target_test.py +++ b/infra/cifuzz/fuzz_target_test.py @@ -18,7 +18,9 @@ import sys import tempfile import unittest import unittest.mock -import urllib + +import parameterized +from pyfakefs import fake_filesystem_unittest # Pylint has issue importing utils which is why error suppression is required. # pylint: disable=wrong-import-position @@ -26,8 +28,6 @@ import urllib sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) import fuzz_target -import utils - # NOTE: This integration test relies on # https://github.com/google/oss-fuzz/tree/master/projects/example project. EXAMPLE_PROJECT = 'example' @@ -35,45 +35,80 @@ EXAMPLE_PROJECT = 'example' # An example fuzzer that triggers an error. EXAMPLE_FUZZER = 'example_crash_fuzzer' -# Location of files used for testing. -TEST_FILES_PATH = os.path.join(os.path.dirname(os.path.abspath(__file__)), - 'test_files') +# The return value of a successful call to utils.execute. +EXECUTE_SUCCESS_RETVAL = ('', '', 0) + +# The return value of a failed call to utils.execute. +EXECUTE_FAILURE_RETVAL = ('', '', 1) -class IsReproducibleUnitTest(unittest.TestCase): - """Test is_reproducible function in the fuzz_target module.""" +# TODO(metzman): Use patch from test_libs/helpers.py in clusterfuzz so that we +# don't need to accept this as an argument in every test method. +@unittest.mock.patch('utils.get_container_name', return_value='container') +class IsReproducibleUnitTest(fake_filesystem_unittest.TestCase): + """Tests the is_reproducible method in the fuzz_target.FuzzTarget class.""" def setUp(self): """Sets up dummy fuzz target to test is_reproducible method.""" - self.test_target = fuzz_target.FuzzTarget('/example/path', 10, + self.fuzz_target_path = '/example/path' + self.testcase_path = '/testcase' + self.test_target = fuzz_target.FuzzTarget(self.fuzz_target_path, + fuzz_target.REPRODUCE_ATTEMPTS, '/example/outdir') - def test_with_reproducible(self): - """Tests that a is_reproducible will return true if crash is detected.""" - test_all_success = [(0, 0, 1)] * 10 - with unittest.mock.patch.object(utils, - 'execute', - side_effect=test_all_success) as patch: - self.assertTrue( - self.test_target.is_reproducible(TEST_FILES_PATH, '/not/exist')) - self.assertEqual(1, patch.call_count) - - test_one_success = [(0, 0, 0)] * 9 + [(0, 0, 1)] - with unittest.mock.patch.object(utils, - 'execute', - side_effect=test_one_success) as patch: + def test_reproducible(self, _): + """Tests that is_reproducible returns True if crash is detected and that + is_reproducible uses the correct command to reproduce a crash.""" + self._set_up_fakefs() + all_repro = [EXECUTE_FAILURE_RETVAL] * fuzz_target.REPRODUCE_ATTEMPTS + with unittest.mock.patch('utils.execute', + side_effect=all_repro) as mocked_execute: + result = self.test_target.is_reproducible(self.testcase_path, + self.fuzz_target_path) + mocked_execute.assert_called_once_with([ + 'docker', 'run', '--rm', '--privileged', '--volumes-from', + 'container', '-e', 'OUT=/example', '-e', + 'TESTCASE=' + self.testcase_path, '-t', + 'gcr.io/oss-fuzz-base/base-runner', 'reproduce', 'path', '-runs=100' + ]) + self.assertTrue(result) + self.assertEqual(1, mocked_execute.call_count) + + def _set_up_fakefs(self): + """Helper to setup pyfakefs and add important files to the fake + filesystem.""" + self.setUpPyfakefs() + self.fs.create_file(self.fuzz_target_path) + self.fs.create_file(self.testcase_path) + + def test_flaky(self, _): + """Tests that is_reproducible returns True if crash is detected on the last + attempt.""" + self._set_up_fakefs() + last_time_repro = [EXECUTE_SUCCESS_RETVAL] * 9 + [EXECUTE_FAILURE_RETVAL] + with unittest.mock.patch('utils.execute', + side_effect=last_time_repro) as mocked_execute: self.assertTrue( - self.test_target.is_reproducible(TEST_FILES_PATH, '/not/exist')) - self.assertEqual(10, patch.call_count) - - def test_with_not_reproducible(self): - """Tests that a is_reproducible will return False if crash not detected.""" - test_all_fail = [(0, 0, 0)] * 10 - with unittest.mock.patch.object(utils, 'execute', - side_effect=test_all_fail) as patch: - self.assertFalse( - self.test_target.is_reproducible(TEST_FILES_PATH, '/not/exist')) - self.assertEqual(10, patch.call_count) + self.test_target.is_reproducible(self.testcase_path, + self.fuzz_target_path)) + self.assertEqual(fuzz_target.REPRODUCE_ATTEMPTS, + mocked_execute.call_count) + + def test_nonexistent_fuzzer(self, _): + """Tests that is_reproducible raises an error if it could not attempt + reproduction because the fuzzer doesn't exist.""" + with self.assertRaises(fuzz_target.ReproduceError): + self.test_target.is_reproducible(self.testcase_path, '/non-existent-path') + + def test_unreproducible(self, _): + """Tests that is_reproducible returns False for a crash that did not + reproduce.""" + all_unrepro = [EXECUTE_SUCCESS_RETVAL] * fuzz_target.REPRODUCE_ATTEMPTS + self._set_up_fakefs() + with unittest.mock.patch('utils.execute', side_effect=all_unrepro): + result = self.test_target.is_reproducible(self.testcase_path, + self.fuzz_target_path) + self.assertFalse(result) class GetTestCaseUnitTest(unittest.TestCase): @@ -84,7 +119,7 @@ class GetTestCaseUnitTest(unittest.TestCase): self.test_target = fuzz_target.FuzzTarget('/example/path', 10, '/example/outdir') - def test_with_valid_error_string(self): + def test_valid_error_string(self): """Tests that get_test_case returns the correct test case give an error.""" test_case_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'test_files', @@ -95,8 +130,8 @@ class GetTestCaseUnitTest(unittest.TestCase): parsed_test_case, '/example/outdir/crash-ad6700613693ef977ff3a8c8f4dae239c3dde6f5') - def test_with_invalid_error_string(self): - """Tests that get_test_case will return None with a bad error string.""" + def test_invalid_error_string(self): + """Tests that get_test_case returns None with a bad error string.""" self.assertIsNone(self.test_target.get_test_case('')) self.assertIsNone(self.test_target.get_test_case(' Example crash string.')) @@ -105,28 +140,26 @@ class DownloadLatestCorpusUnitTest(unittest.TestCase): """Test parse_fuzzer_output function in the cifuzz module.""" def test_download_valid_projects_corpus(self): - """Tests that a vaild fuzz target will return a corpus directory.""" + """Tests that a vaild fuzz target returns a corpus directory.""" with tempfile.TemporaryDirectory() as tmp_dir: test_target = fuzz_target.FuzzTarget('testfuzzer', 3, 'test_out') test_target.project_name = EXAMPLE_PROJECT test_target.target_name = EXAMPLE_FUZZER test_target.out_dir = tmp_dir - with unittest.mock.patch.object(fuzz_target, - 'download_and_unpack_zip', - return_value=tmp_dir) as mock: + with unittest.mock.patch( + 'fuzz_target.download_and_unpack_zip', + return_value=tmp_dir) as mocked_download_and_unpack_zip: test_target.download_latest_corpus() - (url, out_dir), _ = mock.call_args + (url, out_dir), _ = mocked_download_and_unpack_zip.call_args self.assertEqual( - url, - 'https://storage.googleapis.com/example-backup.' \ - 'clusterfuzz-external.appspot.com/corpus/libFuzzer/' \ - 'example_crash_fuzzer/public.zip' - ) + url, 'https://storage.googleapis.com/example-backup.' + 'clusterfuzz-external.appspot.com/corpus/libFuzzer/' + 'example_crash_fuzzer/public.zip') self.assertEqual(out_dir, os.path.join(tmp_dir, 'backup_corpus', EXAMPLE_FUZZER)) def test_download_invalid_projects_corpus(self): - """Tests that a invaild fuzz target will not return None.""" + """Tests that a invaild fuzz target does not return None.""" with tempfile.TemporaryDirectory() as tmp_dir: test_target = fuzz_target.FuzzTarget('testfuzzer', 3, tmp_dir) corpus_path = test_target.download_latest_corpus() @@ -137,53 +170,95 @@ class DownloadLatestCorpusUnitTest(unittest.TestCase): self.assertIsNone(corpus_path) -class CheckReproducibilityAndRegressionUnitTest(unittest.TestCase): - """Test check_reproducibility_and_regression function fuzz_target module.""" +class IsCrashReportableUnitTest(fake_filesystem_unittest.TestCase): + """Test is_crash_reportable method of fuzz_target.FuzzTarget.""" def setUp(self): - """Sets up dummy fuzz target to test is_reproducible method.""" - self.test_target = fuzz_target.FuzzTarget('/example/do_stuff_fuzzer', 100, + """Sets up dummy fuzz target to test is_crash_reportable method.""" + self.fuzz_target_path = '/example/do_stuff_fuzzer' + self.test_target = fuzz_target.FuzzTarget(self.fuzz_target_path, 100, '/example/outdir', 'example') - - def test_with_valid_crash(self): - """Checks to make sure a valid crash returns true.""" - with unittest.mock.patch.object( - fuzz_target.FuzzTarget, 'is_reproducible', - side_effect=[True, False]), tempfile.TemporaryDirectory() as tmp_dir: - self.test_target.out_dir = tmp_dir - self.assertTrue( - self.test_target.check_reproducibility_and_regression( - '/example/crash/testcase')) - - def test_with_invalid_crash(self): - """Checks to make sure an invalid crash returns false.""" - with unittest.mock.patch.object(fuzz_target.FuzzTarget, - 'is_reproducible', - side_effect=[True, True]): - self.assertFalse( - self.test_target.check_reproducibility_and_regression( - '/example/crash/testcase')) - - with unittest.mock.patch.object(fuzz_target.FuzzTarget, - 'is_reproducible', - side_effect=[False, True]): - self.assertFalse( - self.test_target.check_reproducibility_and_regression( - '/example/crash/testcase')) - - with unittest.mock.patch.object(fuzz_target.FuzzTarget, - 'is_reproducible', - side_effect=[False, False]): - self.assertFalse( - self.test_target.check_reproducibility_and_regression( - '/example/crash/testcase')) + self.oss_fuzz_build_path = '/oss-fuzz-build' + self.setUpPyfakefs() + self.fs.create_file(self.fuzz_target_path) + self.oss_fuzz_target_path = os.path.join( + self.oss_fuzz_build_path, os.path.basename(self.fuzz_target_path)) + self.fs.create_file(self.oss_fuzz_target_path) + self.testcase_path = '/testcase' + self.fs.create_file(self.testcase_path, contents='') + + @unittest.mock.patch('logging.info') + def test_new_reproducible_crash(self, mocked_info): + """Tests that a new reproducible crash returns True.""" + + with unittest.mock.patch('fuzz_target.FuzzTarget.is_reproducible', + side_effect=[True, False]): + with tempfile.TemporaryDirectory() as tmp_dir: + self.test_target.out_dir = tmp_dir + self.assertTrue(self.test_target.is_crash_reportable( + self.testcase_path)) + mocked_info.assert_called_with( + 'The crash is reproducible. The crash doesn\'t reproduce ' + 'on old builds. This pull request probably introduced the ' + 'crash.') + + # yapf: disable + @parameterized.parameterized.expand([ + # Reproducible on PR build, but also reproducible on OSS-Fuzz. + ([True, True],), + + # Not reproducible on PR build, but somehow reproducible on OSS-Fuzz. + # Unlikely to happen in real world except if test is flaky. + ([False, True],), + + # Not reproducible on PR build, and not reproducible on OSS-Fuzz. + ([False, False],), + ]) + # yapf: enable + def test_invalid_crash(self, is_reproducible_retvals): + """Tests that a nonreportable crash causes the method to return False.""" + with unittest.mock.patch('fuzz_target.FuzzTarget.is_reproducible', + side_effect=is_reproducible_retvals): + + with unittest.mock.patch('fuzz_target.FuzzTarget.download_oss_fuzz_build', + return_value=self.oss_fuzz_build_path): + self.assertFalse( + self.test_target.is_crash_reportable(self.testcase_path)) + + @unittest.mock.patch('logging.info') + @unittest.mock.patch('fuzz_target.FuzzTarget.is_reproducible', + return_value=[True]) + def test_reproducible_no_oss_fuzz_target(self, _, mocked_info): + """Tests that is_crash_reportable returns True when a crash repros on the + PR build but the target is not in the OSS-Fuzz build (usually because it + is new).""" + os.remove(self.oss_fuzz_target_path) + + def is_reproducible_side_effect(_, target_path): + if os.path.dirname(target_path) == self.oss_fuzz_build_path: + raise fuzz_target.ReproduceError() + return True + + with unittest.mock.patch( + 'fuzz_target.FuzzTarget.is_reproducible', + side_effect=is_reproducible_side_effect) as mocked_is_reproducible: + with unittest.mock.patch('fuzz_target.FuzzTarget.download_oss_fuzz_build', + return_value=self.oss_fuzz_build_path): + self.assertTrue(self.test_target.is_crash_reportable( + self.testcase_path)) + mocked_is_reproducible.assert_any_call(self.testcase_path, + self.oss_fuzz_target_path) + mocked_info.assert_called_with( + 'Crash is reproducible. Could not run OSS-Fuzz build of ' + 'target to determine if this pull request introduced crash. ' + 'Assuming this pull request introduced crash.') class GetLatestBuildVersionUnitTest(unittest.TestCase): """Test the get_latest_build_version function in the fuzz_target module.""" def test_get_valid_project(self): - """Checks the latest build can be retrieved from gcs.""" + """Tests that the latest build can be retrieved from GCS.""" test_target = fuzz_target.FuzzTarget('/example/path', 10, '/example/outdir', 'example') latest_build = test_target.get_lastest_build_version() @@ -192,7 +267,7 @@ class GetLatestBuildVersionUnitTest(unittest.TestCase): self.assertTrue('address' in latest_build) def test_get_invalid_project(self): - """Checks the latest build will return None when project doesn't exist.""" + """Tests that the latest build returns None when project doesn't exist.""" test_target = fuzz_target.FuzzTarget('/example/path', 10, '/example/outdir', 'not-a-proj') self.assertIsNone(test_target.get_lastest_build_version()) @@ -204,23 +279,22 @@ class DownloadOSSFuzzBuildDirIntegrationTests(unittest.TestCase): """Test the download_oss_fuzz_build in function in the fuzz_target module.""" def test_single_download(self): - """Checks that the build directory was only downloaded once.""" + """Tests that the build directory was only downloaded once.""" with tempfile.TemporaryDirectory() as tmp_dir: test_target = fuzz_target.FuzzTarget('/example/do_stuff_fuzzer', 10, tmp_dir, 'example') latest_version = test_target.get_lastest_build_version() - with unittest.mock.patch.object( - fuzz_target.FuzzTarget, - 'get_lastest_build_version', - return_value=latest_version) as mock_build_version: + with unittest.mock.patch( + 'fuzz_target.FuzzTarget.get_lastest_build_version', + return_value=latest_version) as mocked_get_latest_build_version: for _ in range(5): oss_fuzz_build_path = test_target.download_oss_fuzz_build() - self.assertEqual(1, mock_build_version.call_count) + self.assertEqual(1, mocked_get_latest_build_version.call_count) self.assertIsNotNone(oss_fuzz_build_path) self.assertTrue(os.listdir(oss_fuzz_build_path)) def test_get_valid_project(self): - """Checks the latest build can be retrieved from gcs.""" + """Tests the latest build can be retrieved from GCS.""" with tempfile.TemporaryDirectory() as tmp_dir: test_target = fuzz_target.FuzzTarget('/example/do_stuff_fuzzer', 10, tmp_dir, 'example') @@ -229,7 +303,7 @@ class DownloadOSSFuzzBuildDirIntegrationTests(unittest.TestCase): self.assertTrue(os.listdir(oss_fuzz_build_path)) def test_get_invalid_project(self): - """Checks the latest build will return None when project doesn't exist.""" + """Tests the latest build returns None when project doesn't exist.""" with tempfile.TemporaryDirectory() as tmp_dir: test_target = fuzz_target.FuzzTarget('/example/do_stuff_fuzzer', 10, tmp_dir) @@ -239,7 +313,7 @@ class DownloadOSSFuzzBuildDirIntegrationTests(unittest.TestCase): self.assertIsNone(test_target.download_oss_fuzz_build()) def test_invalid_build_dir(self): - """Checks the download will return None when out_dir doesn't exist.""" + """Tests the download returns None when out_dir doesn't exist.""" test_target = fuzz_target.FuzzTarget('/example/do_stuff_fuzzer', 10, 'not/a/dir', 'example') self.assertIsNone(test_target.download_oss_fuzz_build()) @@ -250,8 +324,8 @@ class DownloadAndUnpackZipUnitTests(unittest.TestCase): def test_bad_zip_download(self): """Tests download_and_unpack_zip returns none when a bad zip is passed.""" - with tempfile.TemporaryDirectory() as tmp_dir, unittest.mock.patch.object( - urllib.request, 'urlretrieve', return_value=True): + with tempfile.TemporaryDirectory() as tmp_dir, unittest.mock.patch( + 'urllib.request.urlretrieve', return_value=True): file_handle = open(os.path.join(tmp_dir, 'url_tmp.zip'), 'w') file_handle.write('Test file.') file_handle.close() diff --git a/infra/dev-requirements.txt b/infra/dev-requirements.txt deleted file mode 100644 index 55e3b504..00000000 --- a/infra/dev-requirements.txt +++ /dev/null @@ -1,5 +0,0 @@ -# Requirements for submitting code changes to infra/ (needed by presubmit.py). -pylint==2.4.4 -yapf==0.28.0 -PyYAML==5.1 - diff --git a/infra/presubmit.py b/infra/presubmit.py index f8cf0110..a4d61bdc 100755 --- a/infra/presubmit.py +++ b/infra/presubmit.py @@ -334,16 +334,16 @@ def run_tests(): for file in get_changed_files(): changed_dirs.add(os.path.dirname(file)) - # TODO(metzman): This approach for running tests is probably flawed since - # tests can fail even if their directory isn't changed. Figure out if it is - # needed (to save time) and remove it if it isn't. + # TODO(metzman): This approach for running tests is flawed since tests can + # fail even if their directory isn't changed. Figure out if it is needed (to + # save time) and remove it if it isn't. suite_list = [] for change_dir in changed_dirs: suite_list.append(unittest.TestLoader().discover(change_dir, pattern='*_test.py')) - full_suite = unittest.TestSuite(suite_list) - result = unittest.TextTestRunner().run(full_suite) - return not result.failures + suite = unittest.TestSuite(suite_list) + result = unittest.TextTestRunner().run(suite) + return not result.failures and not result.errors def main(): diff --git a/infra/travis/requirements.txt b/infra/travis/requirements.txt index 37917bb2..2ddcb47d 100644 --- a/infra/travis/requirements.txt +++ b/infra/travis/requirements.txt @@ -1 +1,6 @@ +# Requirements for submitting code changes to infra/ (needed by presubmit.py). +pylint==2.4.4 +yapf==0.28.0 PyYAML==5.1 +pyfakefs==3.7.1 +parameterized==0.7.4 |