Bug 1546611 - [raptor] Fix None checks when validating test manifests. r=davehunt
authorHenrik Skupin <mail@hskupin.info>
Fri, 26 Apr 2019 12:48:02 +0000
changeset 530302 d4742516f3b5db4195e53e96de143a0a6eef0bbb
parent 530301 836d02c18692c8037a94c319054b6104e0380208
child 530303 e02e1b1425934e95738946a7b7602d341844c011
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdavehunt
bugs1546611
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1546611 - [raptor] Fix None checks when validating test manifests. r=davehunt Differential Revision: https://phabricator.services.mozilla.com/D28642
testing/raptor/raptor/manifest.py
testing/raptor/test/test_manifest.py
--- a/testing/raptor/raptor/manifest.py
+++ b/testing/raptor/raptor/manifest.py
@@ -10,21 +10,31 @@ from manifestparser import TestManifest
 from mozlog import get_proxy_logger
 from utils import transform_platform
 
 here = os.path.abspath(os.path.dirname(__file__))
 raptor_ini = os.path.join(here, 'raptor.ini')
 tests_dir = os.path.join(here, 'tests')
 LOG = get_proxy_logger(component="raptor-manifest")
 
-required_settings = ['apps', 'type', 'page_cycles', 'test_url', 'measure',
-                     'unit', 'lower_is_better', 'alert_threshold']
+required_settings = [
+    'alert_threshold',
+    'apps',
+    'lower_is_better',
+    'measure',
+    'page_cycles',
+    'test_url',
+    'type',
+    'unit',
+]
 
-playback_settings = ['playback_pageset_manifest',
-                     'playback_recordings']
+playback_settings = [
+    'playback_pageset_manifest',
+    'playback_recordings',
+]
 
 
 def filter_app(tests, values):
     for test in tests:
         if values["app"] in test['apps']:
             yield test
 
 
@@ -57,49 +67,50 @@ def get_browser_test_list(browser_app, r
 def validate_test_ini(test_details):
     # validate all required test details were found in the test INI
     valid_settings = True
 
     for setting in required_settings:
         # measure setting not required for benchmark type tests
         if setting == 'measure' and test_details['type'] == 'benchmark':
             continue
-        if setting not in test_details:
+        if test_details.get(setting) is None:
             # if page-cycles is not specified, it's ok as long as browser-cycles is there
             if setting == "page-cycles" and test_details.get('browser_cycles') is not None:
                 continue
             valid_settings = False
             LOG.error("ERROR: setting '%s' is required but not found in %s"
                       % (setting, test_details['manifest']))
 
     test_details.setdefault("page_timeout", 30000)
 
     # if playback is specified, we need more playback settings
-    if 'playback' in test_details:
+    if test_details.get('playback') is not None:
         for setting in playback_settings:
-            if setting not in test_details:
+            if test_details.get(setting) is None:
                 valid_settings = False
                 LOG.error("ERROR: setting '%s' is required but not found in %s"
                           % (setting, test_details['manifest']))
 
     # if 'alert-on' is specified, we need to make sure that the value given is valid
     # i.e. any 'alert_on' values must be values that exist in the 'measure' ini setting
-    if 'alert_on' in test_details:
+    if test_details.get('alert_on') is not None:
 
         # support with or without spaces, i.e. 'measure = fcp, loadtime' or '= fcp,loadtime'
         # convert to a list; and remove any spaces
         test_details['alert_on'] = [_item.strip() for _item in test_details['alert_on'].split(',')]
 
         # now make sure each alert_on value provided is valid
         for alert_on_value in test_details['alert_on']:
             if alert_on_value not in test_details['measure']:
                 LOG.error("ERROR: The 'alert_on' value of '%s' is not valid because "
                           "it doesn't exist in the 'measure' test setting!"
                           % alert_on_value)
                 valid_settings = False
+
     return valid_settings
 
 
 def write_test_settings_json(args, test_details, oskey):
     # write test settings json file with test details that the control
     # server will provide for the web ext
     test_url = transform_platform(test_details['test_url'], oskey)
 
--- a/testing/raptor/test/test_manifest.py
+++ b/testing/raptor/test/test_manifest.py
@@ -18,106 +18,134 @@ sys.path.insert(1, mozharness_dir)
 
 raptor_dir = os.path.join(os.path.dirname(here), 'raptor')
 sys.path.insert(0, raptor_dir)
 
 from manifest import get_browser_test_list, validate_test_ini, get_raptor_test_list
 
 
 # some test details (test INIs)
-VALID_MANIFESTS = [{'apps': 'firefox',
-                    'type': 'pageload',
-                    'page_cycles': 25,
-                    'test_url': 'http://www.test-url/goes/here',
-                    'measure': 'fnbpaint, fcp',
-                    'alert_on': 'fcp',
-                    'unit': 'ms',
-                    'lower_is_better': True,
-                    'alert_threshold': 2.0,
-                    'playback': 'mitmproxy',
-                    'playback_binary_manifest': 'binary.manifest',
-                    'playback_pageset_manifest': 'pageset.manifest',
-                    'playback_recordings': 'recorded_site.mp',
-                    'manifest': 'valid_details_0'},
-                   {'apps': 'chrome',
-                    'type': 'benchmark',
-                    'page_cycles': 5,
-                    'test_url': 'http://www.test-url/goes/here',
-                    'measure': 'fcp',
-                    'unit': 'score',
-                    'lower_is_better': False,
-                    'alert_threshold': 2.0,
-                    'manifest': 'valid_details_1'},
-                   {'apps': 'chromium',
-                    'type': 'benchmark',
-                    'page_cycles': 5,
-                    'test_url': 'http://www.test-url/goes/here',
-                    'measure': 'fcp',
-                    'unit': 'score',
-                    'lower_is_better': False,
-                    'alert_threshold': 2.0,
-                    'manifest': 'valid_details_1'},
-                   {'apps': 'geckoview',
-                    'type': 'pageload',
-                    'cold': True,
-                    'browser_cycles': 10,
-                    'page_cycles': 1,
-                    'test_url': 'http://www.test-url/goes/here',
-                    'measure': 'fcp',
-                    'unit': 'score',
-                    'lower_is_better': False,
-                    'alert_threshold': 2.0,
-                    'manifest': 'valid_details_2'}]
+VALID_MANIFESTS = [{
+    # page load test with local playback
+    'alert_on': 'fcp',
+    'alert_threshold': 2.0,
+    'apps': 'firefox',
+    'lower_is_better': True,
+    'manifest': 'valid_details_0',
+    'measure': 'fnbpaint, fcp',
+    'page_cycles': 25,
+    'playback': 'mitmproxy',
+    'playback_binary_manifest': 'binary.manifest',
+    'playback_pageset_manifest': 'pageset.manifest',
+    'playback_recordings': 'recorded_site.mp',
+    'test_url': 'http://www.test-url/goes/here',
+    'type': 'pageload',
+    'unit': 'ms',
+}, {
+    # test optional settings with None
+    'alert_threshold': 2.0,
+    'apps': 'firefox',
+    'lower_is_better': True,
+    'manifest': 'valid_details_1',
+    'measure': 'fnbpaint, fcb',
+    'page_cycles': 25,
+    'test_url': 'http://www.test-url/goes/here',
+    'type': 'pageload',
+    'unit': 'ms',
 
-INVALID_MANIFESTS = [{'apps': 'firefox',
-                      'type': 'pageload',
-                      'page_cycles': 25,
-                      'test_url': 'http://www.test-url/goes/here',
-                      'unit': 'ms',
-                      'lower_is_better': True,
-                      'alert_threshold': 2.0,
-                      'playback': 'mitmproxy',
-                      'playback_binary_manifest': 'binary.manifest',
-                      'playback_pageset_manifest': 'pageset.manifest',
-                      'playback_recordings': 'recorded_site.mp',
-                      'manifest': 'invalid_details_0'},
-                     {'apps': 'chrome',
-                      'type': 'pageload',
-                      'page_cycles': 25,
-                      'test_url': 'http://www.test-url/goes/here',
-                      'measure': 'fnbpaint, fcp',
-                      'unit': 'ms',
-                      'lower_is_better': True,
-                      'alert_threshold': 2.0,
-                      'playback': 'mitmproxy',
-                      'manifest': 'invalid_details_1'},
-                     {'apps': 'chromium',
-                      'type': 'pageload',
-                      'page_cycles': 25,
-                      'test_url': 'http://www.test-url/goes/here',
-                      'measure': 'fnbpaint, fcp',
-                      'unit': 'ms',
-                      'lower_is_better': True,
-                      'alert_threshold': 2.0,
-                      'playback': 'mitmproxy',
-                      'manifest': 'invalid_details_1'},
-                     {'apps': 'firefox',
-                      'type': 'pageload',
-                      'page_cycles': 25,
-                      'test_url': 'http://www.test-url/goes/here',
-                      'measure': 'fnbpaint, fcp',
-                      'alert_on': 'nope',
-                      'unit': 'ms',
-                      'lower_is_better': True,
-                      'alert_threshold': 2.0,
-                      'playback': 'mitmproxy',
-                      'playback_binary_manifest': 'binary.manifest',
-                      'playback_pageset_manifest': 'pageset.manifest',
-                      'playback_recordings': 'recorded_site.mp',
-                      'manifest': 'invalid_details_2'}]
+    'alert_on': None,
+    'playback': None,
+}, {
+    # page load test for geckoview
+    'alert_threshold': 2.0,
+    'apps': 'geckoview',
+    'browser_cycles': 10,
+    'cold': True,
+    'lower_is_better': False,
+    'manifest': 'valid_details_2',
+    'measure': 'fcp',
+    'page_cycles': 1,
+    'test_url': 'http://www.test-url/goes/here',
+    'type': 'pageload',
+    'unit': 'score',
+}, {
+    # benchmark test for chrome
+    'alert_threshold': 2.0,
+    'apps': 'chrome',
+    'lower_is_better': False,
+    'manifest': 'valid_details_1',
+    'measure': 'fcp',
+    'page_cycles': 5,
+    'test_url': 'http://www.test-url/goes/here',
+    'type': 'benchmark',
+    'unit': 'score',
+}, {
+    # benchmark test for chromium
+    'alert_threshold': 2.0,
+    'apps': 'chromium',
+    'lower_is_better': False,
+    'manifest': 'valid_details_1',
+    'measure': 'fcp',
+    'page_cycles': 5,
+    'test_url': 'http://www.test-url/goes/here',
+    'type': 'benchmark',
+    'unit': 'score',
+}]
+
+INVALID_MANIFESTS = [{
+    'alert_threshold': 2.0,
+    'apps': 'firefox',
+    'lower_is_better': True,
+    'manifest': 'invalid_details_0',
+    'page_cycles': 25,
+    'playback': 'mitmproxy',
+    'playback_binary_manifest': 'binary.manifest',
+    'playback_pageset_manifest': 'pageset.manifest',
+    'playback_recordings': 'recorded_site.mp',
+    'test_url': 'http://www.test-url/goes/here',
+    'type': 'pageload',
+    'unit': 'ms',
+}, {
+    'alert_threshold': 2.0,
+    'apps': 'chrome',
+    'lower_is_better': True,
+    'manifest': 'invalid_details_1',
+    'measure': 'fnbpaint, fcp',
+    'page_cycles': 25,
+    'playback': 'mitmproxy',
+    'test_url': 'http://www.test-url/goes/here',
+    'type': 'pageload',
+    'unit': 'ms',
+}, {
+    'alert_threshold': 2.0,
+    'apps': 'chromium',
+    'lower_is_better': True,
+    'manifest': 'invalid_details_1',
+    'measure': 'fnbpaint, fcp',
+    'page_cycles': 25,
+    'playback': 'mitmproxy',
+    'test_url': 'http://www.test-url/goes/here',
+    'type': 'pageload',
+    'unit': 'ms',
+}, {
+    'alert_on': 'nope',
+    'alert_threshold': 2.0,
+    'apps': 'firefox',
+    'lower_is_better': True,
+    'manifest': 'invalid_details_2',
+    'measure': 'fnbpaint, fcp',
+    'page_cycles': 25,
+    'playback': 'mitmproxy',
+    'playback_binary_manifest': 'binary.manifest',
+    'playback_pageset_manifest': 'pageset.manifest',
+    'playback_recordings': 'recorded_site.mp',
+    'test_url': 'http://www.test-url/goes/here',
+    'type': 'pageload',
+    'unit': 'ms',
+}]
 
 
 @pytest.mark.parametrize('app', ['firefox', 'chrome', 'chromium', 'geckoview', 'refbrow', 'fenix'])
 def test_get_browser_test_list(app):
     test_list = get_browser_test_list(app, run_local=True)
     assert len(test_list) > 0