Attachment 345984 Details for Bug 188134 – Proposed patch

[patch] Proposed patch

ews_trigger_test.patch (text/plain), 8.84 KB, created by Aakash Jain on 2018-07-27 18:28:11 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Aakash Jain

Created: 2018-07-27 18:28:11 PDT

Size: 8.84 KB

patch

obsolete

>Index: Tools/ChangeLog
>===================================================================
>--- Tools/ChangeLog	(revision 234341)
>+++ Tools/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2018-07-27  Aakash Jain  <aakash_jain@apple.com>
>+
>+        [ews-build] loadConfig should ensure that the triggers are valid
>+        https://bugs.webkit.org/show_bug.cgi?id=188134
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * BuildSlaveSupport/ews-build/loadConfig.py:
>+        (checkValidBuilder): Added a check to ensure that the builder doesn't refernce non-existing scheduler.
>+        (checkValidSchedulers): Ensures that the scheduler is not un-used.
>+        * BuildSlaveSupport/ews-build/loadConfig_unittest.py: Updated unit-tests.
>+
> 2018-07-27  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         More unreviewed GTK test gardening
>Index: Tools/BuildSlaveSupport/ews-build/loadConfig.py
>===================================================================
>--- Tools/BuildSlaveSupport/ews-build/loadConfig.py	(revision 234341)
>+++ Tools/BuildSlaveSupport/ews-build/loadConfig.py	(working copy)
>@@ -40,7 +40,8 @@ STEP_NAME_LENGTH_LIMIT = 50
> def loadBuilderConfig(c, use_localhost_worker=False, master_prefix_path='./'):
>     config = json.load(open(os.path.join(master_prefix_path, 'config.json')))
>     passwords = json.load(open(os.path.join(master_prefix_path, 'passwords.json')))
>-    checkWorkersAndBuildersForConsistency(config['workers'], config['builders'])
>+    checkWorkersAndBuildersForConsistency(config, config['workers'], config['builders'])
>+    checkValidSchedulers(config, config['schedulers'])
> 
>     c['workers'] = [Worker(worker['name'], passwords.get(worker['name'], 'password')) for worker in config['workers']]
>     if use_localhost_worker:
>@@ -93,7 +94,7 @@ def checkValidWorker(worker):
>         raise Exception('Worker {} does not have platform defined.'.format(worker['name']))
> 
> 
>-def checkValidBuilder(builder):
>+def checkValidBuilder(config, builder):
>     if not builder:
>         raise Exception('Builder is None or Empty.')
> 
>@@ -115,8 +116,33 @@ def checkValidBuilder(builder):
>     if not builder.get('platform'):
>         raise Exception('Builder {} does not have platform defined.'.format(builder['name']))
> 
>+    for trigger in builder.get('triggers') or []:
>+        if not doesTriggerExist(config, trigger):
>+            raise Exception('Trigger: {} in builder {} does not exist in list of Trigerrable schedulers.'.format(trigger, builder['name']))
> 
>-def checkWorkersAndBuildersForConsistency(workers, builders):
>+
>+def checkValidSchedulers(config, schedulers):
>+    for scheduler in config.get('schedulers') or []:
>+        if scheduler.get('type') == 'Triggerable':
>+            if not isTriggerUsedByAnyBuilder(config, scheduler['name']):
>+                raise Exception('Trigger: {} is not used by any builder in config.json'.format(scheduler['name']))
>+
>+
>+def doesTriggerExist(config, trigger):
>+    for scheduler in config.get('schedulers') or []:
>+        if scheduler['name'] == trigger:
>+            return True
>+    return False
>+
>+
>+def isTriggerUsedByAnyBuilder(config, trigger):
>+    for builder in config.get('builders'):
>+        if trigger in (builder.get('triggers') or []):
>+            return True
>+    return False
>+
>+
>+def checkWorkersAndBuildersForConsistency(config, workers, builders):
>     def _find_worker_with_name(workers, worker_name):
>         for worker in workers:
>             if worker['name'] == worker_name:
>@@ -127,7 +153,7 @@ def checkWorkersAndBuildersForConsistenc
>         checkValidWorker(worker)
> 
>     for builder in builders:
>-        checkValidBuilder(builder)
>+        checkValidBuilder(config, builder)
>         for worker_name in builder['workernames']:
>             worker = _find_worker_with_name(workers, worker_name)
>             if worker is None:
>Index: Tools/BuildSlaveSupport/ews-build/loadConfig_unittest.py
>===================================================================
>--- Tools/BuildSlaveSupport/ews-build/loadConfig_unittest.py	(revision 234341)
>+++ Tools/BuildSlaveSupport/ews-build/loadConfig_unittest.py	(working copy)
>@@ -88,42 +88,47 @@ class TestcheckValidWorker(unittest.Test
> class TestcheckValidBuilder(unittest.TestCase):
>     def test_invalid_builder(self):
>         with self.assertRaises(Exception) as context:
>-            loadConfig.checkValidBuilder({})
>+            loadConfig.checkValidBuilder({}, {})
>         self.assertEqual(context.exception.args, ('Builder is None or Empty.',))
> 
>     def test_builder_with_missing_name(self):
>         with self.assertRaises(Exception) as context:
>-            loadConfig.checkValidBuilder({'platform': 'mac-sierra'})
>+            loadConfig.checkValidBuilder({}, {'platform': 'mac-sierra'})
>         self.assertEqual(context.exception.args, ('Builder "{\'platform\': \'mac-sierra\'}" does not have name defined.',))
> 
>     def test_builder_with_invalid_identifier(self):
>         with self.assertRaises(Exception) as context:
>-            loadConfig.checkValidBuilder({'name': 'mac-wk2(test)'})
>+            loadConfig.checkValidBuilder({}, {'name': 'mac-wk2(test)'})
>         self.assertEqual(context.exception.args, ('Builder name mac-wk2(test) is not a valid buildbot identifier.',))
> 
>     def test_builder_with_extra_long_name(self):
>         longName = 'a' * 71
>         with self.assertRaises(Exception) as context:
>-            loadConfig.checkValidBuilder({'name': longName})
>+            loadConfig.checkValidBuilder({}, {'name': longName})
>         self.assertEqual(context.exception.args, ('Builder name {} is longer than maximum allowed by Buildbot (70 characters).'.format(longName),))
> 
>     def test_builder_with_invalid_configuration(self):
>         with self.assertRaises(Exception) as context:
>-            loadConfig.checkValidBuilder({'name': 'mac-wk2', 'configuration': 'asan'})
>+            loadConfig.checkValidBuilder({}, {'name': 'mac-wk2', 'configuration': 'asan'})
>         self.assertEqual(context.exception.args, ('Invalid configuration: asan for builder: mac-wk2',))
> 
>     def test_builder_with_missing_factory(self):
>         with self.assertRaises(Exception) as context:
>-            loadConfig.checkValidBuilder({'name': 'mac-wk2', 'configuration': 'release'})
>+            loadConfig.checkValidBuilder({}, {'name': 'mac-wk2', 'configuration': 'release'})
>         self.assertEqual(context.exception.args, ('Builder mac-wk2 does not have factory defined.',))
> 
>+    def test_builder_with_missing_scheduler(self):
>+        with self.assertRaises(Exception) as context:
>+            loadConfig.checkValidBuilder({}, {'name': 'mac-wk2', 'configuration': 'release', 'factory': 'WK2Factory', 'platform': 'mac-sierra', 'triggers': ['api-tests-mac-ews']})
>+        self.assertEqual(context.exception.args, ('Trigger: api-tests-mac-ews in builder mac-wk2 does not exist in list of Trigerrable schedulers.',))
>+
>     def test_builder_with_missing_platform(self):
>         with self.assertRaises(Exception) as context:
>-            loadConfig.checkValidBuilder({'name': 'mac-wk2', 'configuration': 'release', 'factory': 'WK2Factory'})
>+            loadConfig.checkValidBuilder({}, {'name': 'mac-wk2', 'configuration': 'release', 'factory': 'WK2Factory'})
>         self.assertEqual(context.exception.args, ('Builder mac-wk2 does not have platform defined.',))
> 
>     def test_valid_builder(self):
>-        loadConfig.checkValidBuilder({'name': 'mac-wk2', 'configuration': 'release', 'factory': 'WK2Factory', 'platform': 'mac-sierra'})
>+        loadConfig.checkValidBuilder({}, {'name': 'mac-wk2', 'configuration': 'release', 'factory': 'WK2Factory', 'platform': 'mac-sierra'})
> 
> 
> class TestcheckWorkersAndBuildersForConsistency(unittest.TestCase):
>@@ -135,16 +140,16 @@ class TestcheckWorkersAndBuildersForCons
> 
>     def test_checkWorkersAndBuildersForConsistency(self):
>         with self.assertRaises(Exception) as context:
>-            loadConfig.checkWorkersAndBuildersForConsistency([], [self.WK2Builder])
>+            loadConfig.checkWorkersAndBuildersForConsistency({}, [], [self.WK2Builder])
>         self.assertEqual(context.exception.args, ('Builder mac-wk2 has worker ews101, which is not defined in workers list!',))
> 
>     def test_checkWorkersAndBuildersForConsistency1(self):
>         with self.assertRaises(Exception) as context:
>-            loadConfig.checkWorkersAndBuildersForConsistency([self.ews101, self.ews102], [self.WK2Builder])
>+            loadConfig.checkWorkersAndBuildersForConsistency({}, [self.ews101, self.ews102], [self.WK2Builder])
>         self.assertEqual(context.exception.args, ('Builder mac-wk2 is for platform mac-sierra, but has worker ews102 for platform ios-11!',))
> 
>     def test_success(self):
>-        loadConfig.checkWorkersAndBuildersForConsistency([self.ews101, {'name': 'ews102', 'platform': 'mac-sierra'}], [self.WK2Builder])
>+        loadConfig.checkWorkersAndBuildersForConsistency({}, [self.ews101, {'name': 'ews102', 'platform': 'mac-sierra'}], [self.WK2Builder])
> 
> 
> if __name__ == '__main__':

Actions: View | Formatted Diff | Diff

Attachments on bug 188134: 345984 | 345989