Add more validation (#1029)

* Add more validation

Add validation to presubmit.py and run_experiment.py to ensure:
1. That experiments are only run with one type of benchmark.
2. That experiments are only run with valid benchmarks.
3. Valid benchmarks must have a valid type.
4. To pass presubmit, benchmarks must have a valid type.

* fmt everywhere

* fix test

Author: jonathanmetzman

analysis/benchmark_results.py

@@ -19,7 +19,7 @@
 from analysis import data_utils
 from analysis import coverage_data_utils
 from analysis import stat_tests
-from common import benchmark_config
+from common import benchmark_utils
 
 
 # pylint: disable=too-many-public-methods, too-many-arguments
@@ -71,15 +71,7 @@ def type(self):
 
         Raises ValueError in case of invalid benchmark type in the config.
         """
-        try:
-            benchmark_type = benchmark_config.get_config(self.name).get('type')
-        except Exception:  # pylint: disable=broad-except
-            return 'code'
-        if not benchmark_type:
-            return 'code'
-        if benchmark_type not in ['code', 'bug']:
-            raise ValueError('Invalid benchmark type: ' + benchmark_type)
-        return benchmark_type
+        return benchmark_utils.get_type(self.name)
 
     @property
     def _relevant_column(self):

analysis/test_experiment_results.py

@@ -12,12 +12,14 @@
 # See the License for the specific language governing permissions andsss
 # limitations under the License.
 """Tests for experiment_results.py"""
+from unittest import mock
 
 from analysis import experiment_results
 from analysis import test_data_utils
 
 
-def test_linkify_fuzzer_names_in_ranking():
+@mock.patch('common.benchmark_config.get_config', return_value={})
+def test_linkify_fuzzer_names_in_ranking(_):
     """Tests turning fuzzer names into links."""
     experiment_df = test_data_utils.create_experiment_data()
     results = experiment_results.ExperimentResults(experiment_df,

common/benchmark_utils.py

@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 """Code for dealing with benchmarks."""
+import enum
 import os
 import re
 
@@ -27,6 +28,17 @@
 BENCHMARKS_DIR = os.path.join(utils.ROOT_DIR, 'benchmarks')
 
 
+class BenchmarkType(str, enum.Enum):
+    """Benchmark type."""
+    CODE = 'code'
+    BUG = 'bug'
+
+
+# pytype: disable=missing-parameter
+BENCHMARK_TYPE_STRS = {benchmark_type.value for benchmark_type in BenchmarkType}
+# pytype: enable=missing-parameter
+
+
 def get_fuzz_target(benchmark):
     """Returns the fuzz target of |benchmark|"""
     return benchmark_config.get_config(benchmark)['fuzz_target']
@@ -37,6 +49,12 @@ def get_project(benchmark):
     return benchmark_config.get_config(benchmark)['project']
 
 
+def get_type(benchmark):
+    """Returns the type of |benchmark|"""
+    return benchmark_config.get_config(benchmark).get('type',
+                                                      BenchmarkType.CODE.value)
+
+
 def get_runner_image_url(experiment, benchmark, fuzzer, docker_registry):
     """Get the URL of the docker runner image for fuzzing the benchmark with
     fuzzer."""
@@ -64,6 +82,16 @@ def validate_name(benchmark):
     return True
 
 
+def validate_type(benchmark):
+    """Returns True if |benchmark| has a valid type."""
+    benchmark_type = get_type(benchmark)
+    if benchmark_type not in BENCHMARK_TYPE_STRS:
+        logs.error('%s has an invalid benchmark type %s, must be one of %s',
+                   benchmark, benchmark_type, BENCHMARK_TYPE_STRS)
+        return False
+    return True
+
+
 def validate(benchmark):
     """Returns True if |benchmark| is a valid fuzzbench benchmark."""
     if not validate_name(benchmark):
@@ -73,6 +101,7 @@ def validate(benchmark):
         logs.error('%s must have a benchmark.yaml.', benchmark)
         return False
 
+    # Validate config file can be parsed.
     try:
         get_fuzz_target(benchmark)
     except yaml.parser.ParserError:
@@ -84,7 +113,8 @@ def validate(benchmark):
                    benchmark)
         return False
 
-    return True
+    # Validate type.
+    return validate_type(benchmark)
 
 
 def get_all_benchmarks():

common/test_benchmark_utils.py

@@ -12,6 +12,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 """Tests for benchmark_utils.py"""
+from unittest import mock
+
 import pytest
 
 from common import benchmark_utils
@@ -56,3 +58,26 @@ def test_validate_name_invalid(benchmark_name):
 def test_validate_name_valid(benchmark_name):
     """Tests that validate_name returns True for a valid benchmark name."""
     assert benchmark_utils.validate_name(benchmark_name)
+
+
+@mock.patch('common.benchmark_utils.get_type', return_value='other-type')
+def test_validate_type_invalid(_):
+    """Tests that validate_type returns False for an invalid type."""
+    assert not benchmark_utils.validate_type('benchmark')
+
+
+@pytest.mark.parametrize(('benchmark_type',), [
+    ('code',),
+    ('bug',),
+])
+def test_validate_type_valid(benchmark_type):
+    """Tests that validate_type returns True for an valid type."""
+    with mock.patch('common.benchmark_utils.get_type',
+                    return_value=benchmark_type):
+        assert benchmark_utils.validate_type('benchmark')
+
+
+@mock.patch('common.benchmark_config.get_config', return_value={})
+def test_get_default_type(_):
+    """Tests that get_type returns the correct default value."""
+    assert benchmark_utils.get_type('benchmark') == 'code'

experiment/run_experiment.py

@@ -151,10 +151,23 @@ def get_directories(parent_dir):
 
 def validate_benchmarks(benchmarks: List[str]):
     """Parses and validates list of benchmarks."""
+    benchmark_types = set()
     for benchmark in set(benchmarks):
         if benchmarks.count(benchmark) > 1:
             raise Exception('Benchmark "%s" is included more than once.' %
                             benchmark)
+        # Validate benchmarks here. It's possible someone might run an
+        # experiment without going through presubmit. Better to catch an invalid
+        # benchmark than see it in production.
+        if not benchmark_utils.validate(benchmark):
+            raise Exception('Benchmark "%s" is invalid.' % benchmark)
+
+        benchmark_types.add(benchmark_utils.get_type(benchmark))
+
+    if (benchmark_utils.BenchmarkType.CODE.value in benchmark_types and
+            benchmark_utils.BenchmarkType.BUG.value in benchmark_types):
+        raise Exception(
+            'Cannot mix bug benchmarks with code coverage benchmarks.')
 
 
 def validate_fuzzer(fuzzer: str):

fuzzbench/test_e2e/test_e2e_run.py

@@ -59,9 +59,9 @@ def test_jobs_dependency(self, experiment_config, redis_connection):  # pylint:
                     assert jobs[dep].ended_at <= jobs[name].started_at
 
     def test_all_jobs_finished_successfully(
-            self,
-            experiment_config,  # pylint: disable=redefined-outer-name
-            redis_connection):  # pylint: disable=redefined-outer-name
+        self,
+        experiment_config,  # pylint: disable=redefined-outer-name
+        redis_connection):  # pylint: disable=redefined-outer-name
         """Tests all jobs finished successully."""
         all_images = docker_images.get_images_to_build(
             experiment_config['fuzzers'], experiment_config['benchmarks'])

service/automatic_run_experiment.py

@@ -23,8 +23,9 @@
 import sys
 from typing import Optional
 
-from common import logs
+from common import benchmark_utils
 from common import fuzzer_utils
+from common import logs
 from common import utils
 from common import yaml_utils
 from database import models
@@ -247,7 +248,7 @@ def run_requested_experiment(dry_run):
     fuzzers = requested_experiment['fuzzers']
 
     benchmark_type = requested_experiment.get('type')
-    if benchmark_type == 'bug':
+    if benchmark_type == benchmark_utils.BenchmarkType.BUG.value:
         benchmarks = BUG_BENCHMARKS
     else:
         benchmarks = CODE_BENCHMARKS

test_libs/utils.py

@@ -25,9 +25,8 @@ def mock_popen_ctx_mgr(*args, **kwargs):
         yield mocked_popen
 
 
-def create_mock_popen(output=bytes('', 'utf-8'),
-                      err=bytes('', 'utf-8'),
-                      returncode=0):
+def create_mock_popen(
+        output=bytes('', 'utf-8'), err=bytes('', 'utf-8'), returncode=0):
     """Creates a mock subprocess.Popen."""
 
     class MockPopen: