From c75d1b362f2c21fd0c66cbec2eae6d1195d99142 Mon Sep 17 00:00:00 2001 From: jonathanmetzman <31354670+jonathanmetzman@users.noreply.github.com> Date: Mon, 2 Aug 2021 12:37:37 -0700 Subject: [PATCH] [cifuzz] Create validate method on BaseConfig (#6135) * [cifuzz] Create validate method on BaseConfig Use it to validate that either OSS_FUZZ_PROJECT_NAME or BUILD_INTEGRATION_PATH is set. Also use it to validate that workspace is set (rather than duplicate code). Add tests. * Use env var hack to bypass valdiation * fix * fix * fmt * fix * tmp * fix --- infra/cifuzz/build_fuzzers_entrypoint.py | 4 ---- infra/cifuzz/config_utils.py | 30 +++++++++++++++++++++--- infra/cifuzz/config_utils_test.py | 29 +++++++++++++++++++++++ infra/cifuzz/docker_test.py | 4 +++- infra/cifuzz/run_fuzzers_entrypoint.py | 4 ---- infra/cifuzz/run_fuzzers_test.py | 1 - infra/cifuzz/test_helpers.py | 12 +++++----- infra/presubmit.py | 7 +++++- 8 files changed, 71 insertions(+), 20 deletions(-) diff --git a/infra/cifuzz/build_fuzzers_entrypoint.py b/infra/cifuzz/build_fuzzers_entrypoint.py index 4acbb3622..c191ca38e 100644 --- a/infra/cifuzz/build_fuzzers_entrypoint.py +++ b/infra/cifuzz/build_fuzzers_entrypoint.py @@ -37,10 +37,6 @@ def build_fuzzers_entrypoint(): # The default return code when an error occurs. returncode = 1 - if not config.workspace: - logging.error('This script needs to be run within Github actions.') - return returncode - if not build_fuzzers.build_fuzzers(config): logging.error('Error building fuzzers for (commit: %s, pr_ref: %s).', config.commit_sha, config.pr_ref) diff --git a/infra/cifuzz/config_utils.py b/infra/cifuzz/config_utils.py index 6162a2b40..121a68b80 100644 --- a/infra/cifuzz/config_utils.py +++ b/infra/cifuzz/config_utils.py @@ -88,6 +88,10 @@ def _get_language(): # pylint: disable=too-few-public-methods,too-many-instance-attributes +class ConfigurationError(Exception): + """Error for invalid configuration.""" + + class BaseConfig: """Object containing constant configuration for CIFuzz.""" @@ -108,10 +112,8 @@ class BaseConfig: self.dry_run = _is_dry_run() self.sanitizer = _get_sanitizer() - # TODO(ochang): Error out if both oss_fuzz and build_integration_path is not - # set. - self.build_integration_path = os.getenv('BUILD_INTEGRATION_PATH') + self.build_integration_path = os.getenv('BUILD_INTEGRATION_PATH') self.language = _get_language() event_path = os.getenv('GITHUB_EVENT_PATH') self.is_github = bool(event_path) @@ -124,6 +126,28 @@ class BaseConfig: self.git_store_branch_coverage = os.environ.get('GIT_STORE_BRANCH_COVERAGE', self.git_store_branch) + # TODO(metzman): Fix tests to create valid configurations and get rid of + # CIFUZZ_TEST here and in presubmit.py. + if not self.validate() and not os.getenv('CIFUZZ_TEST'): + raise ConfigurationError('Invalid Configuration.') + + def validate(self): + """Returns False if the configuration is invalid.""" + # Do validation here so that unittests don't need to make a fully-valid + # config. + if (self.build_integration_path is None and + self.oss_fuzz_project_name is None): + logging.error('Must set OSS_FUZZ_PROJECT_NAME if OSS-Fuzz user. ' + 'Otherwise must set BUILD_INTEGRATION_PATH. ' + 'Neither is set.') + return False + + if not self.workspace: + logging.error('Must set GITHUB_WORKSPACE.') + return False + + return True + @property def is_internal(self): """Returns True if this is an OSS-Fuzz project.""" diff --git a/infra/cifuzz/config_utils_test.py b/infra/cifuzz/config_utils_test.py index bce2bb9fc..2865479cf 100644 --- a/infra/cifuzz/config_utils_test.py +++ b/infra/cifuzz/config_utils_test.py @@ -14,6 +14,7 @@ """Module for getting the configuration CIFuzz needs to run.""" import os import unittest +from unittest import mock import config_utils import test_helpers @@ -56,6 +57,34 @@ class BaseConfigTest(unittest.TestCase): config = self._create_config() self.assertFalse(config.is_coverage) + @mock.patch('logging.error') + def test_validate_oss_fuzz_project_name_or_build_integration_path( + self, mocked_error): + """Tests that validate returns False if neither OSS_FUZZ_PROJECT_NAME or + BUILD_INTEGRATION_PATH is set.""" + os.environ['GITHUB_WORKSPACE'] = '/workspace' + config = self._create_config() + self.assertFalse(config.validate()) + mocked_error.assert_called_with( + 'Must set OSS_FUZZ_PROJECT_NAME if OSS-Fuzz user. ' + 'Otherwise must set BUILD_INTEGRATION_PATH. ' + 'Neither is set.') + + @mock.patch('logging.error') + def test_validate_no_workspace(self, mocked_error): + """Tests that validate returns False if GITHUB_WORKSPACE isn't set.""" + os.environ['OSS_FUZZ_PROJECT_NAME'] = 'example' + config = self._create_config() + self.assertFalse(config.validate()) + mocked_error.assert_called_with('Must set GITHUB_WORKSPACE.') + + def test_validate(self): + """Tests that validate returns True if config is valid.""" + os.environ['OSS_FUZZ_PROJECT_NAME'] = 'example' + os.environ['GITHUB_WORKSPACE'] = '/workspace' + config = self._create_config() + self.assertTrue(config.validate()) + class BuildFuzzersConfigTest(unittest.TestCase): """Tests for BuildFuzzersConfig.""" diff --git a/infra/cifuzz/docker_test.py b/infra/cifuzz/docker_test.py index 68effb5a5..8f46da3fb 100644 --- a/infra/cifuzz/docker_test.py +++ b/infra/cifuzz/docker_test.py @@ -17,9 +17,11 @@ from unittest import mock import config_utils import docker +import test_helpers CONTAINER_NAME = 'example-container' -config = config_utils.RunFuzzersConfig() +config = test_helpers.create_run_config(oss_fuzz_project_name='project', + workspace='/workspace') config.workspace = '/workspace' WORKSPACE = config_utils.Workspace(config) SANITIZER = 'example-sanitizer' diff --git a/infra/cifuzz/run_fuzzers_entrypoint.py b/infra/cifuzz/run_fuzzers_entrypoint.py index 73286766d..f8f6c417d 100644 --- a/infra/cifuzz/run_fuzzers_entrypoint.py +++ b/infra/cifuzz/run_fuzzers_entrypoint.py @@ -52,10 +52,6 @@ def run_fuzzers_entrypoint(): # Sets the default return code on error to success. returncode = 0 - if not config.workspace: - logging.error('This script needs to be run within Github actions.') - return returncode - delete_unneeded_docker_images(config) # Run the specified project's fuzzers from the build. result = run_fuzzers.run_fuzzers(config) diff --git a/infra/cifuzz/run_fuzzers_test.py b/infra/cifuzz/run_fuzzers_test.py index ddbaf4f9b..2916678ed 100644 --- a/infra/cifuzz/run_fuzzers_test.py +++ b/infra/cifuzz/run_fuzzers_test.py @@ -368,7 +368,6 @@ class CoverageReportIntegrationTest(unittest.TestCase): run_config = test_helpers.create_run_config( fuzz_seconds=FUZZ_SECONDS, workspace=workspace, - oss_fuzz_project_name=EXAMPLE_PROJECT, sanitizer=self.SANITIZER, run_fuzzers_mode='coverage', is_github=True, diff --git a/infra/cifuzz/test_helpers.py b/infra/cifuzz/test_helpers.py index 3e87f7a72..705657958 100644 --- a/infra/cifuzz/test_helpers.py +++ b/infra/cifuzz/test_helpers.py @@ -22,19 +22,19 @@ from unittest import mock import config_utils -def _create_config(config_cls, **kwargs): +@mock.patch('config_utils._is_dry_run', return_value=True) +@mock.patch('config_utils.get_project_src_path', return_value=None) +@mock.patch('os.path.basename', return_value=None) +def _create_config(config_cls, _, __, ___, **kwargs): """Creates a config object from |config_cls| and then sets every attribute that is a key in |kwargs| to the corresponding value. Asserts that each key in |kwargs| is an attribute of config.""" - with mock.patch('os.path.basename', return_value=None), mock.patch( - 'config_utils.get_project_src_path', - return_value=None), mock.patch('config_utils._is_dry_run', - return_value=True): + with mock.patch('config_utils.BaseConfig.validate', return_value=True): config = config_cls() - for key, value in kwargs.items(): assert hasattr(config, key), 'Config doesn\'t have attribute: ' + key setattr(config, key, value) + return config diff --git a/infra/presubmit.py b/infra/presubmit.py index 41f74e605..f97569290 100755 --- a/infra/presubmit.py +++ b/infra/presubmit.py @@ -403,7 +403,12 @@ def run_nonbuild_tests(parallel): command.extend(['-n', 'auto']) command += list(relevant_dirs) print('Running non-build tests.') - return subprocess.run(command, check=False).returncode == 0 + + # TODO(metzman): Get rid of this once config_utils stops using it. + env = os.environ.copy() + env['CIFUZZ_TEST'] = '1' + + return subprocess.run(command, check=False, env=env).returncode == 0 def run_tests(_=None, parallel=False, skip_build_tests=False):