Reimplemented tree_for_master, which now uses gatekeeper config files to map master URLs to the tre…

infra/tools/builder_alerts/test/gatekeeper_extras_test.py

Index: infra/tools/builder_alerts/test/gatekeeper_extras_test.py
diff --git a/infra/tools/builder_alerts/test/gatekeeper_extras_test.py b/infra/tools/builder_alerts/test/gatekeeper_extras_test.py
new file mode 100644
index 0000000000000000000000000000000000000000..5889be8bf62d7db46f4c0bec99e17468d3229fd4
--- /dev/null
+++ b/infra/tools/builder_alerts/test/gatekeeper_extras_test.py
@@ -0,0 +1,154 @@
+# Copyright 2014 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import mock
+import unittest
+
+from infra.tools.builder_alerts import gatekeeper_extras
+
+class GatekeeperExtrasTest(unittest.TestCase):

[tandrii(chromium), 2014/09/04 14:21:06]

nit: PEP8 two lines before top-level classes?

[Sergiy Byelozyorov, 2014/09/05 12:59:07]

Done.

+  def patch(self, target, new):
+    patcher = mock.patch(target, new)
+    patcher.start()
+    self.addCleanup(patcher.stop)
+
+  def test_excluded_builders(self):
+    self.assertEqual(gatekeeper_extras.excluded_builders([{}]), set())
+    self.assertEqual(gatekeeper_extras.excluded_builders([{'*': {}}]), set())
+    self.assertEqual(
+        gatekeeper_extras.excluded_builders(
+            [{'*': {
+                'excluded_builders': set(['test_builder1', 'test_builder2'])
+            }}]),
+        set(['test_builder1', 'test_builder2']))
+
+  def test_apply_gatekeeper_rules(self):
+    self.patch('infra.tools.builder_alerts.gatekeeper_extras.excluded_builders',
+               lambda config: ['Ignored'])
+    self.patch('infra.tools.builder_alerts.gatekeeper_extras.would_close_tree',
+               lambda config, builder, step: True)
+    self.patch('infra.tools.builder_alerts.gatekeeper_extras.tree_for_master',
+               lambda master_url, trees_config: 'test-tree')
+
+    gatekeeper_cfg = {'http://build.chromium.org/p/chromium': {'key': 'value'}}
+    gatekeeper_trees_cfg = {}
+    alerts = [
+        {'master_url': 'http://build.chromium.org/p/project.without.config',
+         'builder_name': 'Linux',
+         'step_name': 'test_xyz'},
+        {'master_url': 'http://build.chromium.org/p/chromium',
+         'builder_name': 'Ignored',
+         'step_name': 'bot_update'},
+        {'master_url': 'http://build.chromium.org/p/chromium',
+         'builder_name': 'Win',
+         'step_name': 'bot_update'}
+    ]
+
+    filtered_alerts = gatekeeper_extras.apply_gatekeeper_rules(
+        alerts, gatekeeper_cfg, gatekeeper_trees_cfg)
+
+    self.assertEqual(len(filtered_alerts), 2)
+    self.assertIn({'master_url': 'http://build.chromium.org/p/chromium',
+                   'builder_name': 'Win',
+                   'step_name': 'bot_update',
+                   'would_close_tree': True,
+                   'tree': 'test-tree'}, filtered_alerts)
+    self.assertIn(
+        {'master_url': 'http://build.chromium.org/p/project.without.config',
+         'builder_name': 'Linux',
+         'step_name': 'test_xyz'},
+        filtered_alerts)
+
+  def test_tree_for_master(self):

[agable, 2014/09/04 14:22:31]

I think this test would be better served by breaking it up into several tests,
and using a much smaller gatekeeper_trees dict for each. 
test_tree_for_master_returns_tree_name,
test_tree_for_master_falls_back_to_master_name,
etc.

Tests should test behaviors, not functions.

[Sergiy Byelozyorov, 2014/09/05 12:59:07]

Done.

+    master_name_from_url = mock.patch(
+        'infra.tools.builder_alerts.buildbot.master_name_from_url',
+        lambda url: url.split('/')[-1])
+    master_name_from_url.start()
+    self.addCleanup(master_name_from_url.stop)
+
+    gatekeeper_trees = {
+        "blink": {
+            "build-db": "blink_build_db.json",
+            "masters": [
+                "https://build.chromium.org/p/chromium.webkit"
+            ],
+            "open-tree": True,
+            "password-file": ".blink_status_password",
+            "revision-properties": "got_revision_cp,got_webkit_revision_cp",
+            "set-status": True,
+            "status-url": "https://blink-status.appspot.com",
+            "track-revisions": True
+        },
+        "chromium": {
+            "build-db": "waterfall_build_db.json",
+            "masters": [
+                "https://build.chromium.org/p/chromium",
+                "https://build.chromium.org/p/chromium.chrome",
+                "https://build.chromium.org/p/chromium.chromiumos",
+                "https://build.chromium.org/p/chromium.gpu",
+                "https://build.chromium.org/p/chromium.linux",
+                "https://build.chromium.org/p/chromium.mac",
+                "https://build.chromium.org/p/chromium.memory",
+                "https://build.chromium.org/p/chromium.win"
+            ],
+            "open-tree": True,
+            "password-file": ".status_password",
+            "revision-properties": "got_revision_cp",
+            "set-status": True,
+            "status-url": "https://chromium-status.appspot.com",
+            "track-revisions": True
+        },
+        "non-closers": {
+            "masters": [
+                "https://build.chromium.org/p/chromium.lkgr",
+                "https://build.chromium.org/p/chromium.perf",
+                "https://build.chromium.org/p/client.libvpx"
+            ]
+        }
+    }
+
+    self.assertEqual('chromium', gatekeeper_extras.tree_for_master(
+        'https://build.chromium.org/p/chromium.gpu', gatekeeper_trees))
+    self.assertEqual('blink', gatekeeper_extras.tree_for_master(
+        'https://build.chromium.org/p/chromium.webkit', gatekeeper_trees))
+    self.assertEqual('non-closers', gatekeeper_extras.tree_for_master(
+        'https://build.chromium.org/p/chromium.lkgr', gatekeeper_trees))
+    self.assertEqual('foo.bar', gatekeeper_extras.tree_for_master(
+        'https://build.chromium.org/p/foo.bar', gatekeeper_trees))
+
+  def test_fetch_master_urls(self):
+    class MockArgs:
+      def __init__(self, master_filter):
+        self.master_filter = master_filter
+
+    gatekeeper = {'test_master1': {}, 'test_master2': {}, 'filtered_master': {}}
+    self.assertEqual(
+        set(gatekeeper_extras.fetch_master_urls(gatekeeper,
+                                                MockArgs('filtered'))),
+        set(['test_master1', 'test_master2']))
+    self.assertEqual(
+        set(gatekeeper_extras.fetch_master_urls(gatekeeper, MockArgs(None))),
+        set(['test_master1', 'test_master2', 'filtered_master']))
+
+  def test_would_close_tree(self):

[agable, 2014/09/04 14:22:31]

This should definitely be broken into multiple tests, one per behavior.

[Sergiy Byelozyorov, 2014/09/05 12:59:07]

Done.

+    self.assertFalse(gatekeeper_extras.would_close_tree(
+        [{'*': {'close_tree': False}}], None, 'test_step'))
+    self.assertFalse(gatekeeper_extras.would_close_tree(
+        [{'test_builder': {'excluded_steps': ['test_step']}}],
+        'test_builder', 'test_step'))
+    self.assertFalse(gatekeeper_extras.would_close_tree(
+        [{'test_builder': {'closing_steps': set(['other_step'])}}],
+        'test_builder', 'test_step'))
+    self.assertFalse(gatekeeper_extras.would_close_tree(
+        [{'test_builder': {}}], 'test_builder', 'test_step'))
+    self.assertTrue(gatekeeper_extras.would_close_tree(
+        [{'test_builder': {'closing_steps': set(['test_step'])}}],
+        'test_builder', 'test_step'))
+    self.assertTrue(gatekeeper_extras.would_close_tree(
+        [{'test_builder': {'closing_steps': set(['*'])}}],
+        'test_builder', 'test_step'))
+
+
+if __name__ == '__main__':
+  unittest.main()