Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1020103006:
Add feature to run perf try jobs across all platforms on tryserver.chromium.perf. (Closed)
DescriptionAdd feature to run perf try jobs across all platforms on tryserver.chromium.perf.
This provides a list of new browser to run perf try jobs on tryserver.chromium.perf:
try-all: For tryjobs on all bisect bots (android, mac, win, winx64, linux).
trybot-all-android: tryjobs on all android bisect bots.
trybot-all-win: tryjobs on all windows bisect bots (win, win x64).
trybot-all-mac: tryjobs on all mac bisect bots.
trybot-all-linux: tryjobs on all linux bisect bots.
BUG=467124
Committed: https://crrev.com/b895186446956d9a912c477db31b212c365798f4
Cr-Commit-Position: refs/heads/master@{#322064}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : #
Total comments: 22
Patch Set 5 : #
Total comments: 8
Patch Set 6 : #
Messages
Total messages: 14 (3 generated)
prasadv@chromium.org changed reviewers: + prasadv@chromium.org, qyearsley@chromium.org, simonhatch@chromium.org, sullivan@chromium.org
Try jobs with different trybot browser types: Multiple bot tryjobs trybot-all: https://codereview.chromium.org/1023273002 try-all-android: https://codereview.chromium.org/1021053004 try-all-win: https://codereview.chromium.org/1020373002 try-all-mac https://codereview.chromium.org/1025023002 try-all-linux: https://codereview.chromium.org/1027783004 Single tryjobs try-win https://codereview.chromium.org/1026883002 try-winx64 https://codereview.chromium.org/1028723003 try-android-nexus10: https://codereview.chromium.org/1026893002 try-mac: https://codereview.chromium.org/1028683003
lgtm Thanks for doing this!
Looks good, I just took a quick look so far, so my review is not very deep. Also waiting for Simon's review, right? https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py (right): https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:24: EXCLUDED_BOTS = ['win_xp_perf_bisect', 'linux_perf_tester', 1. If you're using a 4-space hanging indent, the first item should also go on the next line. 2. Even though it takes up more vertical space, I think putting one item per line is slightly easier to read. 3. Since you're only checking membership in EXCLUDED_BOTS, and order doesn't matter, you can use a set. So, it could be: EXCLUDED_BOTS = { 'android_perf_bisect_builder' 'linux_perf_bisect_builder', 'mac_perf_bisect_builder', 'win_perf_bisect_builder', 'win_x64_perf_bisect_builder', 'linux_perf_bisector', 'linux_perf_tester', 'win_xp_perf_bisect', } https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:32: ] The above also applies here, this could be: INCLUDE_BOTS = [ 'trybot-all', 'trybot-all-android', 'trybot-all-linux', 'trybot-all-mac', 'trybot-all-win', ] Also, since this is a list of additional bot names to add to the list, this could be called EXTRA_BOTS or COMBINATION_BOTS or something? https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:246: return SUCCESS Ideally, I think that any logical sub-parts of this function can be extracted out to separate functions, so that this function can be a more high-level function which basically just calls private helper functions. (This would take a bit of time to refactor though.) https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:291: # Perf try jobs do not work on Windows XP This comment is not clear anymore since the context below has changed. Maybe remove it? https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py (right): https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:304: is_blink=False): Style nit: if there are names on the first line, then is_blink=False should be over to the right, aligned with the other args.
PTAL https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py (right): https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:24: EXCLUDED_BOTS = ['win_xp_perf_bisect', 'linux_perf_tester', On 2015/03/20 22:52:37, qyearsley wrote: > 1. If you're using a 4-space hanging indent, the first item should also go on > the next line. > 2. Even though it takes up more vertical space, I think putting one item per > line is slightly easier to read. > 3. Since you're only checking membership in EXCLUDED_BOTS, and order doesn't > matter, you can use a set. So, it could be: > > EXCLUDED_BOTS = { > 'android_perf_bisect_builder' > 'linux_perf_bisect_builder', > 'mac_perf_bisect_builder', > 'win_perf_bisect_builder', > 'win_x64_perf_bisect_builder', > 'linux_perf_bisector', > 'linux_perf_tester', > 'win_xp_perf_bisect', > } Done. https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:32: ] On 2015/03/20 22:52:37, qyearsley wrote: > The above also applies here, this could be: > > INCLUDE_BOTS = [ > 'trybot-all', > 'trybot-all-android', > 'trybot-all-linux', > 'trybot-all-mac', > 'trybot-all-win', > ] > > Also, since this is a list of additional bot names to add to the list, this > could be called EXTRA_BOTS or COMBINATION_BOTS or something? Done. https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:246: return SUCCESS On 2015/03/20 22:52:37, qyearsley wrote: > Ideally, I think that any logical sub-parts of this function can be extracted > out to separate functions, so that this function can be a more high-level > function which basically just calls private helper functions. > > (This would take a bit of time to refactor though.) I agree, but I feel doing refactoring in separate CL makes more sense. https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:291: # Perf try jobs do not work on Windows XP On 2015/03/20 22:52:37, qyearsley wrote: > This comment is not clear anymore since the context below has changed. > Maybe remove it? Done. https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py (right): https://codereview.chromium.org/1020103006/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:304: is_blink=False): On 2015/03/20 22:52:37, qyearsley wrote: > Style nit: if there are names on the first line, then is_blink=False should be > over to the right, aligned with the other args. Done.
https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py (right): https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:54: self._buildernames = _GetBuilderNames(browser_type) For consistency, since "builder" and "names" are treated as two words in the function name, this variable should be called _builder_names. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:142: Returns a perf config parameters as dictionary. Remove "Returns a". Maybe rephrase as "A dictionary with perf config parameters." https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:204: return ERROR This method is quite long because it has to do a lot of error checking. Would it be clearer if some of it could be extracted (lines 186 to 204?) to a helper function called _CheckBranchState or something like that? https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:254: logging.info('Deleted temp branch: telemetry-tryjob') Perhaps the inside of the finally block could be extracted out to be a separate function called _CleanupBranch or something similar. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py (right): https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:77: 'trybot-android-nexus4', 'trybot-mac-10-9'] This would be more readable as: expected_trybots_list = [ 'trybot-all', 'trybot-all-android', 'trybot-all-linux', 'trybot-all-mac', 'trybot-all-win', 'trybot-android-nexus4', 'trybot-mac-10-9' ] https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:80: # pylint: disable=W0212 I don't think that this pylint disable is required. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:102: self.assertEquals('android', browser._target_os) target_os should be a property of browser that you can access. https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... Same below. If you change this, you should be able to remove the pylint disable above. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:103: # pylint: disable=W0212 In general if you have multiple statements which violate some pylint rule and you want to disable it for a block, it's good practice to put an enable after the block of statements. # pylint: disable=W0212 ... ... # pylint: enable=W0212 Same below. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:106: browser._buildernames.get('android')) Align with line above. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:136: ['linux_perf_bisect'], sorted(browser._buildernames.get('linux'))) Extra space. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:185: Extra blank line.
PTAL for Final pass. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py (right): https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:54: self._buildernames = _GetBuilderNames(browser_type) On 2015/03/24 04:58:24, qyearsley wrote: > For consistency, since "builder" and "names" are treated as two words in the > function name, this variable should be called _builder_names. Done. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:142: Returns a perf config parameters as dictionary. On 2015/03/24 04:58:24, qyearsley wrote: > Remove "Returns a". > Maybe rephrase as "A dictionary with perf config parameters." Done. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:204: return ERROR On 2015/03/24 04:58:24, qyearsley wrote: > This method is quite long because it has to do a lot of error checking. > > Would it be clearer if some of it could be extracted (lines 186 to 204?) to a > helper function called _CheckBranchState or something like that? I totally agree, but as mentioned we want differentiate between functionality changes vs refactoring by creating separate CLS. I added TODO to refactor this codes. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder.py:254: logging.info('Deleted temp branch: telemetry-tryjob') On 2015/03/24 04:58:24, qyearsley wrote: > Perhaps the inside of the finally block could be extracted out to be a separate > function called _CleanupBranch or something similar. Added TODO https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py (right): https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:77: 'trybot-android-nexus4', 'trybot-mac-10-9'] On 2015/03/24 04:58:24, qyearsley wrote: > This would be more readable as: > > expected_trybots_list = [ > 'trybot-all', > 'trybot-all-android', > 'trybot-all-linux', > 'trybot-all-mac', > 'trybot-all-win', > 'trybot-android-nexus4', > 'trybot-mac-10-9' > ] Done. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:80: # pylint: disable=W0212 On 2015/03/24 04:58:24, qyearsley wrote: > I don't think that this pylint disable is required. Done. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:102: self.assertEquals('android', browser._target_os) On 2015/03/24 04:58:24, qyearsley wrote: > target_os should be a property of browser that you can access. > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > Same below. If you change this, you should be able to remove the pylint disable > above. Done. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:103: # pylint: disable=W0212 On 2015/03/24 04:58:24, qyearsley wrote: > In general if you have multiple statements which violate some pylint rule and > you want to disable it for a block, it's good practice to put an enable after > the block of statements. > > # pylint: disable=W0212 > ... > ... > # pylint: enable=W0212 > > Same below. agree https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:106: browser._buildernames.get('android')) On 2015/03/24 04:58:24, qyearsley wrote: > Align with line above. Done. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:136: ['linux_perf_bisect'], sorted(browser._buildernames.get('linux'))) On 2015/03/24 04:58:24, qyearsley wrote: > Extra space. Done. https://codereview.chromium.org/1020103006/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:185: On 2015/03/24 04:58:24, qyearsley wrote: > Extra blank line. Done.
Excellent; adding TODOs for refactoring is a good idea I think. LGTM after one more little change in the test. https://codereview.chromium.org/1020103006/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py (right): https://codereview.chromium.org/1020103006/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:133: self.assertEquals('all', browser._target_os) This _target_os can also be changed, and the pylint disable above it can also be removed. https://codereview.chromium.org/1020103006/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:166: self.assertEquals('all', browser._target_os) Same here. https://codereview.chromium.org/1020103006/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:206: self.assertEquals('all', browser._target_os) And here https://codereview.chromium.org/1020103006/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:228: self.assertEquals('all', browser._target_os) And here
https://codereview.chromium.org/1020103006/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py (right): https://codereview.chromium.org/1020103006/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:133: self.assertEquals('all', browser._target_os) On 2015/03/24 18:37:33, qyearsley wrote: > This _target_os can also be changed, and the pylint disable above it can also be > removed. Done. https://codereview.chromium.org/1020103006/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:166: self.assertEquals('all', browser._target_os) On 2015/03/24 18:37:33, qyearsley wrote: > Same here. Done. https://codereview.chromium.org/1020103006/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:206: self.assertEquals('all', browser._target_os) On 2015/03/24 18:37:33, qyearsley wrote: > And here Done. https://codereview.chromium.org/1020103006/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/remote/trybot_browser_finder_unittest.py:228: self.assertEquals('all', browser._target_os) On 2015/03/24 18:37:33, qyearsley wrote: > And here Done.
The CQ bit was checked by prasadv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sullivan@chromium.org, qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/1020103006/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1020103006/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b895186446956d9a912c477db31b212c365798f4 Cr-Commit-Position: refs/heads/master@{#322064} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
