Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2152663003:
Refactor the buildbot module in webkitpy. (Closed)
DescriptionRefactor the buildbot module in webkitpy.
This CL refactors BuildBot so that:
- There is now a results_url method in the BuildBot class (moved from AbstractRebaseliningCommand)
- Redundant functions are removed
- The Builder and Build classes are removed
- Buildbot-related functions in webkitpy.common.config.urls are moved into BuildBot
- Unit test and mock version are updated
The purpose of this is to simplify the code; this is a follow-up to http://crrev.com/2112133002 which removed some unused functionality.
Committed: https://crrev.com/66b942da80d7af5806f50630e55a9800102f43e1
Cr-Commit-Position: refs/heads/master@{#406599}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add comments #Patch Set 3 : Fix test #
Messages
Total messages: 32 (14 generated)
Description was changed from ========== Refactor the buildbot module in webkitpy. This CL refactors BuildBot so that: - There is now a results_url method in the BuildBot class (moved from AbstractRebaseliningCommand) - Redundant functions are removed - Buildbot-related functions in webkitpy.common.config.urls are moved into BuildBot - Unit test and mock version are updated ========== to ========== Refactor the buildbot module in webkitpy. This CL refactors BuildBot so that: - There is now a results_url method in the BuildBot class (moved from AbstractRebaseliningCommand) - Redundant functions are removed - Buildbot-related functions in webkitpy.common.config.urls are moved into BuildBot - Unit test and mock version are updated The purpose of this is to simplify the code; this is a follow-up to http://crrev.com/2112133002 which removed some unused functionality. ==========
qyearsley@chromium.org changed reviewers: + dcampb@google.com, dpranke@chromium.org
Description was changed from ========== Refactor the buildbot module in webkitpy. This CL refactors BuildBot so that: - There is now a results_url method in the BuildBot class (moved from AbstractRebaseliningCommand) - Redundant functions are removed - Buildbot-related functions in webkitpy.common.config.urls are moved into BuildBot - Unit test and mock version are updated The purpose of this is to simplify the code; this is a follow-up to http://crrev.com/2112133002 which removed some unused functionality. ========== to ========== Refactor the buildbot module in webkitpy. This CL refactors BuildBot so that: - There is now a results_url method in the BuildBot class (moved from AbstractRebaseliningCommand) - Redundant functions are removed - The Builder and Build classes are removed - Buildbot-related functions in webkitpy.common.config.urls are moved into BuildBot - Unit test and mock version are updated The purpose of this is to simplify the code; this is a follow-up to http://crrev.com/2112133002 which removed some unused functionality. ==========
qyearsley@chromium.org changed reviewers: + wkorman@chromium.org
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py (right): https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py:47: """ Whats the benefit of not having a __init__ in this class and passing all the arguments as parameters for each function? https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py:77: results_file = None Why is there a Builder object? Would it be beneficial to move all the functions from Builder to BuildBot and pass the arguments as parameters to the functions?
After this change, I noticed that there are two types of methods in this class: 1. Functions to construct URLs for archived results 2. Functions to download archived results, given URLs Functions which just construct URLs don't need to be mocked in unit tests, and functions to download results could be replaced with Web.get_binary. So, maybe it would make more sense if: 1. There is no BuildBot class, and instead just a collection of functions for constructing URLs 2. Host should have a Web object instead of a BuildBot object, and that Web object can used to download results, and mocked out in tests. Does that sound good? https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py (right): https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py:47: """ On 2016/07/15 at 17:33:26, dcampb wrote: > Whats the benefit of not having a __init__ in this class and passing all the arguments as parameters for each function? Good question -- __init__ methods aren't necessary if there's nothing to set up on instance attributes. As it is, instances of this class have no attributes; they only serve as an interface to the functionality provided by the methods. How this class is used currently is that WebKitPatch has an instead of BuildBot, which it uses to access buildbot-related functionality (downloading results for specific builders and builds), and it uses the same BuildBot instance to download results for different builders. So, each time a method here is called it's called with a different argument, so there's no benefit to having an __init__ method that takes that argument and stores it as an attribute. https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py:77: results_file = None On 2016/07/15 at 17:33:26, dcampb wrote: > Why is there a Builder object? Would it be beneficial to move all the functions from Builder to BuildBot and pass the arguments as parameters to the functions? That's what this CL does -- after this change, there should be no BuildBot object.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/15 at 17:50:00, qyearsley wrote: > After this change, I noticed that there are two types of methods in this class: > > 1. Functions to construct URLs for archived results > 2. Functions to download archived results, given URLs > > Functions which just construct URLs don't need to be mocked in unit tests, and functions to download results could be replaced with Web.get_binary. > > So, maybe it would make more sense if: > > 1. There is no BuildBot class, and instead just a collection of functions for constructing URLs > 2. Host should have a Web object instead of a BuildBot object, and that Web object can used to download results, and mocked out in tests. > > Does that sound good? I worry a bit that losing all semantics around the method names and switching tests to just mock Web.get_binary could make test writing/maintenance harder. Like, if we end up in a situation where for some cases we need to do multiple Web.get_binary, or fetch data in some other way. But I'm open to it if you'd like to try it. Would you still want to go forward with this current change?
On 2016/07/18 at 19:58:53, wkorman wrote: > On 2016/07/15 at 17:50:00, qyearsley wrote: > > After this change, I noticed that there are two types of methods in this class: > > > > 1. Functions to construct URLs for archived results > > 2. Functions to download archived results, given URLs > > > > Functions which just construct URLs don't need to be mocked in unit tests, and functions to download results could be replaced with Web.get_binary. > > > > So, maybe it would make more sense if: > > > > 1. There is no BuildBot class, and instead just a collection of functions for constructing URLs > > 2. Host should have a Web object instead of a BuildBot object, and that Web object can used to download results, and mocked out in tests. > > > > Does that sound good? > > I worry a bit that losing all semantics around the method names and switching tests to just mock Web.get_binary could make test writing/maintenance harder. > Like, if we end up in a situation where for some cases we need to do multiple Web.get_binary, or fetch data in some other way. > > But I'm open to it if you'd like to try it. Would you still want to go forward with this current change? That's a good point -- Currently tests which test behavior which requires fetching layout test results just use the mock results in MockBuildBot, and I do think that the current way of doing it (with BuildBot and MockBuildBot classes) is OK. I think I'd like to go forward with this change, and then possibly consider removing the BuildBot class and using Web to download layout test results later.
https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py (right): https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py:62: return '%s/%s' % (RESULTS_URL_BASE, re.sub('[ .()]', '_', builder_name)) What is this re sub doing and why? Perhaps add a comment. https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py:66: return self.builder_results_url_base(builder_name) + "/results/layout-test-results" Is there a doc we can link to in class doc comment that gives an overview of the layout test results json data format? When I work on these scripts I always just have to reverse-engineer it from code and sample data myself, figured worth asking in case you've seen something I haven't and can help future eng.
https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py (right): https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py:62: return '%s/%s' % (RESULTS_URL_BASE, re.sub('[ .()]', '_', builder_name)) On 2016/07/19 at 22:46:27, wkorman wrote: > What is this re sub doing and why? Perhaps add a comment. I think that replacing parentheses, dots and spaces with underscores in builder names for Google Storage directory names was originally done to make the directory names more convenient and URL-friendly; now it has to be done to maintain compatibility with existing directory names. Note, this function was copied over from config/urls.py. Added a docstring. https://codereview.chromium.org/2152663003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py:66: return self.builder_results_url_base(builder_name) + "/results/layout-test-results" On 2016/07/19 at 22:46:27, wkorman wrote: > Is there a doc we can link to in class doc comment that gives an overview of the layout test results json data format? > > When I work on these scripts I always just have to reverse-engineer it from code and sample data myself, figured worth asking in case you've seen something I haven't and can help future eng. The official doc is supposed to be https://www.chromium.org/developers/the-json-test-results-format, I think -- and for layout tests the code that's responsible for the layout test results output is summarize_results in https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp.... I can't see any harm in mentioning this in a comment here. This also seems like something that could be mentioned in layouttestresults.py.
lgtm
The CQ bit was checked by qyearsley@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by qyearsley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2152663003/#ps40001 (title: "Fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Refactor the buildbot module in webkitpy. This CL refactors BuildBot so that: - There is now a results_url method in the BuildBot class (moved from AbstractRebaseliningCommand) - Redundant functions are removed - The Builder and Build classes are removed - Buildbot-related functions in webkitpy.common.config.urls are moved into BuildBot - Unit test and mock version are updated The purpose of this is to simplify the code; this is a follow-up to http://crrev.com/2112133002 which removed some unused functionality. ========== to ========== Refactor the buildbot module in webkitpy. This CL refactors BuildBot so that: - There is now a results_url method in the BuildBot class (moved from AbstractRebaseliningCommand) - Redundant functions are removed - The Builder and Build classes are removed - Buildbot-related functions in webkitpy.common.config.urls are moved into BuildBot - Unit test and mock version are updated The purpose of this is to simplify the code; this is a follow-up to http://crrev.com/2112133002 which removed some unused functionality. Committed: https://crrev.com/66b942da80d7af5806f50630e55a9800102f43e1 Cr-Commit-Position: refs/heads/master@{#406599} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/66b942da80d7af5806f50630e55a9800102f43e1 Cr-Commit-Position: refs/heads/master@{#406599}
Message was sent while issue was closed.
vadimsh@chromium.org changed reviewers: + vadimsh@chromium.org
Message was sent while issue was closed.
Probably broke rebaseline-o-matic: https://build.chromium.org/p/chromium.infra.cron/builders/rebaseline-o-matic/...
Message was sent while issue was closed.
On 2016/07/20 at 17:51:31, vadimsh wrote: > Probably broke rebaseline-o-matic: https://build.chromium.org/p/chromium.infra.cron/builders/rebaseline-o-matic/... Hmm, I think you're right. Reverting now.
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2165983003/ by qyearsley@chromium.org. The reason for reverting is: On 2016/07/20 at 17:51:31, vadimsh wrote: > Probably broke rebaseline-o-matic: https://build.chromium.org/p/chromium.infra.cron/builders/rebaseline-o-matic/.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
