|
|
Created:
6 years, 3 months ago by Sergiy Byelozyorov Modified:
6 years, 3 months ago CC:
chromium-reviews, eseidel, ghost stip (do not use), Jeffrey Yasskin Base URL:
https://chromium.googlesource.com/infra/infra.git@master Project:
infra Visibility:
Public. |
DescriptionReimplemented tree_for_master, which now uses gatekeeper config files to map master URLs to the tree names.
Also added tests to achieve 100% coverage for files which were not previously tested at all.
Depends on https://codereview.chromium.org/514813003.
BUG=406969
R=ojan@chromium.org
Committed: https://chromium.googlesource.com/infra/infra/+/6290b51f8776f84d8b02f09702c1ade911e75a48
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 8
Patch Set 9 : #Patch Set 10 : Added python-mock dependency #Patch Set 11 : Various changes to achieve 100% test coverage #Patch Set 12 : Rebase #
Total comments: 22
Patch Set 13 : Review #Patch Set 14 : Dummy #
Messages
Total messages: 47 (15 generated)
Needs tests. Unfortunately, this code landed in a rush without tests, so we now need to fill in tests for any code we right as we go. Otherwise, just a few minor comments. https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... File infra/tools/builder_alerts/__main__.py (right): https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... infra/tools/builder_alerts/__main__.py:51: alert['tree_name'] = tree_name or buildbot.master_name_from_url(master_url) Nit: can we just call this tree instead of tree_name. I asked for this in the original review, but that got lost in the rush to land it. Eventually, I'd like to change builder_name and step_name to be builder and step as well. https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... infra/tools/builder_alerts/__main__.py:92: print 'Processing gatekeeper trees json: %s' % (gatekeeper_trees_path) These two prints seem unneccessary to me. Lets either remove them or make then logging.debug. https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... File infra/tools/builder_alerts/gatekeeper_extras.py (right): https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:17: if tree_name == 'non-closers': Lets include the non-closers as a tree name for now. Down the road we might decide to use the master name as the tree name in those cases, but this is a start. https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:21: return None Instead of returning None, lets return the last bit of the master_url, e.g. chromium.lkgr. Can do this as a TODO if you'd like.
PTAL https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... File infra/tools/builder_alerts/__main__.py (right): https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... infra/tools/builder_alerts/__main__.py:51: alert['tree_name'] = tree_name or buildbot.master_name_from_url(master_url) On 2014/08/28 03:34:15, ojan-only-code-yellow-reviews wrote: > Nit: can we just call this tree instead of tree_name. I asked for this in the > original review, but that got lost in the rush to land it. Eventually, I'd like > to change builder_name and step_name to be builder and step as well. Done. https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... infra/tools/builder_alerts/__main__.py:92: print 'Processing gatekeeper trees json: %s' % (gatekeeper_trees_path) On 2014/08/28 03:34:15, ojan-only-code-yellow-reviews wrote: > These two prints seem unneccessary to me. Lets either remove them or make then > logging.debug. Done. https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... File infra/tools/builder_alerts/gatekeeper_extras.py (right): https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:17: if tree_name == 'non-closers': On 2014/08/28 03:34:15, ojan-only-code-yellow-reviews wrote: > Lets include the non-closers as a tree name for now. Down the road we might > decide to use the master name as the tree name in those cases, but this is a > start. Done. https://codereview.chromium.org/508873005/diff/130001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:21: return None On 2014/08/28 03:34:15, ojan-only-code-yellow-reviews wrote: > Instead of returning None, lets return the last bit of the master_url, e.g. > chromium.lkgr. Can do this as a TODO if you'd like. Done.
lgtm
The CQ bit was checked by sergiyb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiyb@chromium.org/508873005/150001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: infra_tester on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: infra_tester on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)
The CQ bit was unchecked by sergiyb@chromium.org
Mock library is not available on the bots. Should I just add it to the infra/libs dir?
On 2014/08/29 12:52:03, Sergiy Byelozyorov wrote: > Mock library is not available on the bots. Should I just add it to the > infra/libs dir? Added to third_party folder to avoid pylint warnings. PTAL again
The CQ bit was checked by sergiyb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiyb@chromium.org/508873005/170001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: infra_tester on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: infra_tester on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)
The CQ bit was checked by sergiyb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiyb@chromium.org/508873005/190001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: infra_tester on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: infra_tester on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)
The CQ bit was checked by sergiyb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiyb@chromium.org/508873005/210001
The CQ bit was unchecked by sergiyb@chromium.org
sergiyb@chromium.org changed reviewers: + agable@chromium.org, tandrii@chromium.org
sergiyb@chromium.org changed reviewers: + phajdan.jr@chromium.org - agable@chromium.org
sergiyb@chromium.org changed reviewers: + agable@chromium.org - phajdan.jr@chromium.org
The CQ bit was checked by sergiyb@chromium.org
The CQ bit was unchecked by sergiyb@chromium.org
lgtm with minor nitpicks. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... File infra/tools/builder_alerts/__main__.py (right): https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/__main__.py:66: logging.debug('Processing gatekeeper trees json: %s' % gatekeeper_trees_path) logging accepts *args at the end, and does string formatting automatically only if needed (i.e., logging level <= DEBUG); logging.debug('Processing gatekeeper trees json: %s', gatekeeper_trees_path) will work faster on average. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... File infra/tools/builder_alerts/gatekeeper_extras.py (right): https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:49: # https://chrome-infra-stats.appspot.com/_ah/api#p/stats/v1/stats.masters.list nit: I thought comments should end with period. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... File infra/tools/builder_alerts/test/gatekeeper_extras_test.py (right): https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/test/gatekeeper_extras_test.py:10: class GatekeeperExtrasTest(unittest.TestCase): nit: PEP8 two lines before top-level classes?
https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... File infra/tools/builder_alerts/gatekeeper_extras.py (right): https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:9: import infra.tools.builder_alerts.buildbot Since you only use one thing, could do from infra.tools.builder_alerts.buildbot import master_name_from_url https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:16: """Returns name of the tree for a given master or its name on failure. "Get the name of the tree for a given master url, or the master's name." https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:18: Retuns: Don't really need a "returns" section when the first line already starts with "returns". https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:19: Tree name of master name if the master url is not present in gatekeeper tree name *or* https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:25: return infra.tools.builder_alerts.buildbot.master_name_from_url(master_url) Why is this fallback now necessary? https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... File infra/tools/builder_alerts/test/gatekeeper_extras_test.py (right): https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/test/gatekeeper_extras_test.py:63: def test_tree_for_master(self): 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. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/test/gatekeeper_extras_test.py:134: def test_would_close_tree(self): This should definitely be broken into multiple tests, one per behavior.
Also, rewrite the CL description so that the first line is short enough for Git to like it (~70 characters).
stip@chromium.org changed reviewers: + stip@chromium.org
https://chromiumcodereview.appspot.com/508873005/diff/230001/bootstrap/deps.pyl File bootstrap/deps.pyl (right): https://chromiumcodereview.appspot.com/508873005/diff/230001/bootstrap/deps.p... bootstrap/deps.pyl:27: 'gs': 'ba2b1d5f84448497e14e25922c5e3293f0a91c7e.tar.gz', I've copied mock to infra/third_party/mock Please switch this to rev: '96c2601faa69f6d0ecb931df837632c7776ed4d4'
Patchset #13 (id:250001) has been deleted
PTAL https://codereview.chromium.org/508873005/diff/230001/bootstrap/deps.pyl File bootstrap/deps.pyl (right): https://codereview.chromium.org/508873005/diff/230001/bootstrap/deps.pyl#newc... bootstrap/deps.pyl:27: 'gs': 'ba2b1d5f84448497e14e25922c5e3293f0a91c7e.tar.gz', On 2014/09/04 23:15:40, stip wrote: > I've copied mock to infra/third_party/mock > > Please switch this to rev: '96c2601faa69f6d0ecb931df837632c7776ed4d4' Done. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... File infra/tools/builder_alerts/__main__.py (right): https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/__main__.py:66: logging.debug('Processing gatekeeper trees json: %s' % gatekeeper_trees_path) On 2014/09/04 14:21:06, tandrii wrote: > logging accepts *args at the end, and does string formatting automatically only > if needed (i.e., logging level <= DEBUG); > > logging.debug('Processing gatekeeper trees json: %s', gatekeeper_trees_path) > > will work faster on average. Done. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... File infra/tools/builder_alerts/gatekeeper_extras.py (right): https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:9: import infra.tools.builder_alerts.buildbot On 2014/09/04 14:22:31, agable wrote: > Since you only use one thing, could do > from infra.tools.builder_alerts.buildbot import master_name_from_url Done. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:16: """Returns name of the tree for a given master or its name on failure. On 2014/09/04 14:22:31, agable wrote: > "Get the name of the tree for a given master url, or the master's name." Done. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:18: Retuns: On 2014/09/04 14:22:31, agable wrote: > Don't really need a "returns" section when the first line already starts with > "returns". Done. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:19: Tree name of master name if the master url is not present in gatekeeper On 2014/09/04 14:22:31, agable wrote: > tree name *or* Acknowledged. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:25: return infra.tools.builder_alerts.buildbot.master_name_from_url(master_url) On 2014/09/04 14:22:31, agable wrote: > Why is this fallback now necessary? Master info may not be in a config file. In this case we return master name, so that something shows up in sheriff-o-matic. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/gatekeeper_extras.py:49: # https://chrome-infra-stats.appspot.com/_ah/api#p/stats/v1/stats.masters.list On 2014/09/04 14:21:06, tandrii wrote: > nit: I thought comments should end with period. Yeah, but URL takes entire line and adding a period would require breaking it into two lines... and it was this way before in __main__.py (code just moved from there). https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... File infra/tools/builder_alerts/test/gatekeeper_extras_test.py (right): https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/test/gatekeeper_extras_test.py:10: class GatekeeperExtrasTest(unittest.TestCase): On 2014/09/04 14:21:06, tandrii wrote: > nit: PEP8 two lines before top-level classes? Done. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/test/gatekeeper_extras_test.py:63: def test_tree_for_master(self): On 2014/09/04 14:22:31, agable wrote: > 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. Done. https://codereview.chromium.org/508873005/diff/230001/infra/tools/builder_ale... infra/tools/builder_alerts/test/gatekeeper_extras_test.py:134: def test_would_close_tree(self): On 2014/09/04 14:22:31, agable wrote: > This should definitely be broken into multiple tests, one per behavior. Done.
A note changes to mock.patch in the patchset 13: The test runner expect_tests, which we use for running infra tests, does not execute test cleanups properly like the original unittest library does. Therefore calling self.addCleanup(patcher.stop) had no effect. As a result, some tests were failing flaked whenever the test mocking the method in test ran earlier. To solve this problem, I've chosen to use decorator @mock.patch instead which has its own mechanism for cleanups. Another problem is coverage. It appears that coverage considers the decorator @mock.patcher as a branch and reports missing branch coverage. Aaron's hypothesis was that it is happening because coverage is not loaded when the decorators are being run, however, almost identical decorators on other test method do not report missing coverage. Since this is clearly a bug in coverage, I've chosen to use # pragma: no coverage.
On 2014/09/05 13:06:24, Sergiy Byelozyorov wrote: > A note changes to mock.patch in the patchset 13: > > The test runner expect_tests, which we use for running infra tests, does not > execute test cleanups properly like the original unittest library does. > Therefore calling self.addCleanup(patcher.stop) had no effect. As a result, some > tests were failing flaked whenever the test mocking the method in test ran > earlier. To solve this problem, I've chosen to use decorator @mock.patch instead > which has its own mechanism for cleanups. > > Another problem is coverage. It appears that coverage considers the decorator > @mock.patcher as a branch and reports missing branch coverage. Aaron's > hypothesis was that it is happening because coverage is not loaded when the > decorators are being run, however, almost identical decorators on other test > method do not report missing coverage. Since this is clearly a bug in coverage, > I've chosen to use # pragma: no coverage. lgtm
The CQ bit was checked by sergiyb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiyb@chromium.org/508873005/270001
Message was sent while issue was closed.
This issue passed the CQ. To commit it, remove "COMMIT=false" from the description and try again.
The CQ bit was checked by sergiyb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiyb@chromium.org/508873005/290001
Message was sent while issue was closed.
Committed patchset #14 (id:290001) as 6290b51f8776f84d8b02f09702c1ade911e75a48 |