Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1166123004:
Add simple presubmit check to remoting. (Closed)
DescriptionPrint out a note, during change upload, on running Remoting browser-tests before commiting changes under $src/remoting.
BUG=
Committed: https://crrev.com/44de127d69cd00dfc636e2f8f2cdf42f0e85b7d4
Cr-Commit-Position: refs/heads/master@{#333390}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Include tracking bug # for TODO. #
Total comments: 2
Messages
Total messages: 17 (7 generated)
Patchset #1 (id:1) has been deleted
anandc@chromium.org changed reviewers: + jamiewalch@chromium.org
Hi Jamie, PTAL. Thanks.
kelvinp@chromium.org changed reviewers: + kelvinp@chromium.org
https://codereview.chromium.org/1166123004/diff/20001/remoting/PRESUBMIT.py File remoting/PRESUBMIT.py (right): https://codereview.chromium.org/1166123004/diff/20001/remoting/PRESUBMIT.py#n... remoting/PRESUBMIT.py:12: "https://wiki.corp.google.com/twiki/bin/view/Main/ChromotingWaterfall#Running_on_a_Swarming_bot.") This link won't be accessible for non-googlers. But I guess that is ok.
lgtm https://codereview.chromium.org/1166123004/diff/20001/remoting/PRESUBMIT.py File remoting/PRESUBMIT.py (right): https://codereview.chromium.org/1166123004/diff/20001/remoting/PRESUBMIT.py#n... remoting/PRESUBMIT.py:23: """TODO(anandc): Run browser-tests on the Chromoting waterfall as part of Please create a bug tracking this work and reference it here.
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1166123004/diff/20001/remoting/PRESUBMIT.py File remoting/PRESUBMIT.py (right): https://codereview.chromium.org/1166123004/diff/20001/remoting/PRESUBMIT.py#n... remoting/PRESUBMIT.py:23: """TODO(anandc): Run browser-tests on the Chromoting waterfall as part of On 2015/06/08 22:46:28, Jamie wrote: > Please create a bug tracking this work and reference it here. Done.
The CQ bit was checked by anandc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamiewalch@chromium.org Link to the patchset: https://codereview.chromium.org/1166123004/#ps60001 (title: "Include tracking bug # for TODO.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1166123004/60001
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/44de127d69cd00dfc636e2f8f2cdf42f0e85b7d4 Cr-Commit-Position: refs/heads/master@{#333390}
Message was sent while issue was closed.
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
Message was sent while issue was closed.
Not LGTM. I think we should revert this. People outside of the team make changes in src/remoting often. We shouldn't waste their time by making them understand how to run remoting-specific tests. https://codereview.chromium.org/1166123004/diff/60001/remoting/PRESUBMIT.py File remoting/PRESUBMIT.py (right): https://codereview.chromium.org/1166123004/diff/60001/remoting/PRESUBMIT.py#n... remoting/PRESUBMIT.py:12: "https://wiki.corp.google.com/twiki/bin/view/Main/ChromotingWaterfall#Running_on_a_Swarming_bot.") I don't think you need the . at the end. https://codereview.chromium.org/1166123004/diff/60001/remoting/PRESUBMIT.py#n... remoting/PRESUBMIT.py:12: "https://wiki.corp.google.com/twiki/bin/view/Main/ChromotingWaterfall#Running_on_a_Swarming_bot.") Instructions on that page look long and confusing. I don't think we want to make everybody who changes code in remoting (e.g. a small refactoring or cleanup in base) to read it and figure out how to run the tests. Instead of referring to the page it would be better if we could run all tests automatically. If that's not possible then can this script print a command to run instead of referring to that page.. Also it not right that the page is internal and not available externally.
Message was sent while issue was closed.
On 2015/06/09 23:19:05, Sergey Ulanov wrote: > Not LGTM. I think we should revert this. People outside of the team make changes > in src/remoting often. We shouldn't waste their time by making them understand > how to run remoting-specific tests. > > https://codereview.chromium.org/1166123004/diff/60001/remoting/PRESUBMIT.py > File remoting/PRESUBMIT.py (right): > > https://codereview.chromium.org/1166123004/diff/60001/remoting/PRESUBMIT.py#n... > remoting/PRESUBMIT.py:12: > "https://wiki.corp.google.com/twiki/bin/view/Main/ChromotingWaterfall#Running_on_a_Swarming_bot.") > I don't think you need the . at the end. > > https://codereview.chromium.org/1166123004/diff/60001/remoting/PRESUBMIT.py#n... > remoting/PRESUBMIT.py:12: > "https://wiki.corp.google.com/twiki/bin/view/Main/ChromotingWaterfall#Running_on_a_Swarming_bot.") > Instructions on that page look long and confusing. I don't think we want to make > everybody who changes code in remoting (e.g. a small refactoring or cleanup in > base) to read it and figure out how to run the tests. Instead of referring to > the page it would be better if we could run all tests automatically. If that's > not possible then can this script print a command to run instead of referring to > that page.. > > Also it not right that the page is internal and not available externally. Agree with the overall point. The immediate motivation for this change was to indicate to those making changes under chromoting to be mindful of test breakages. If we can continue to do that as part of code-reviews, that is good. The right solution is to run tests as part of the commit-queue. I've updated https://code.google.com/p/chromium/issues/detail?id=498026 with some info. about getting that in place.
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:60001) has been created in https://codereview.chromium.org/1177633002/ by anandc@chromium.org. The reason for reverting is: Abandoning in favour of a implementing a better longer-term solution.. |
