Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1889483002:
git cl set-commit: for Gerrit + dry-run support. (Closed)
|
Created:
4 years, 8 months ago by tandrii(chromium) Modified:
4 years, 8 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptiongit cl set-commit: for Gerrit + dry-run support.
This provides workaround for not functioning git cl try.
R=sergiyb@chromium.org,andybons@chromium.org,phajdan.jr@chromium.org
BUG=599931
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299900
Patch Set 1 #
Total comments: 2
Patch Set 2 : review #
Total comments: 6
Messages
Total messages: 21 (5 generated)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1889483002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889483002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1889483002/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1889483002/diff/1/git_cl.py#newcode2492 git_cl.py:2492: """Sets the Commit-Queue label assuming canonical CQ config for Gerrit. """ does this need the trailing spaces after "Gerrit."?
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from andybons@chromium.org Link to the patchset: https://codereview.chromium.org/1889483002/#ps20001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1889483002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889483002/20001
https://codereview.chromium.org/1889483002/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1889483002/diff/1/git_cl.py#newcode2492 git_cl.py:2492: """Sets the Commit-Queue label assuming canonical CQ config for Gerrit. """ On 2016/04/13 16:11:22, Bons wrote: > does this need the trailing spaces after "Gerrit."? nope:)
Message was sent while issue was closed.
Description was changed from ========== git cl set-commit: for Gerrit + dry-run support. This provides workaround for not functioning git cl try. R=sergiyb@chromium.org,andybons@chromium.org,phajdan.jr@chromium.org BUG=599931 ========== to ========== git cl set-commit: for Gerrit + dry-run support. This provides workaround for not functioning git cl try. R=sergiyb@chromium.org,andybons@chromium.org,phajdan.jr@chromium.org BUG=599931 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299900 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=299900
Message was sent while issue was closed.
https://codereview.chromium.org/1889483002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1889483002/diff/20001/git_cl.py#newcode1737 git_cl.py:1737: raise NotImplementedError() What about DRY_RUN? https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1316: self.assertEqual(0, git_cl.main(['set-commit'])) also need a test for dry-run here https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1331: self.assertEqual(0, git_cl.main(['set-commit', '-d'])) do you test non-dry-run scenario?
Message was sent while issue was closed.
thanks for review. replies below. https://codereview.chromium.org/1889483002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1889483002/diff/20001/git_cl.py#newcode1737 git_cl.py:1737: raise NotImplementedError() On 2016/04/13 17:15:02, Sergiy Byelozyorov wrote: > What about DRY_RUN? wasn't supported before, so whatever. https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1316: self.assertEqual(0, git_cl.main(['set-commit'])) On 2016/04/13 17:15:03, Sergiy Byelozyorov wrote: > also need a test for dry-run here see above - don't care about dry-run. https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1331: self.assertEqual(0, git_cl.main(['set-commit', '-d'])) On 2016/04/13 17:15:03, Sergiy Byelozyorov wrote: > do you test non-dry-run scenario? no, it's same codepath.
Message was sent while issue was closed.
On 2016/04/13 17:17:16, tandrii(chromium) wrote: > thanks for review. replies below. > > https://codereview.chromium.org/1889483002/diff/20001/git_cl.py > File git_cl.py (right): > > https://codereview.chromium.org/1889483002/diff/20001/git_cl.py#newcode1737 > git_cl.py:1737: raise NotImplementedError() > On 2016/04/13 17:15:02, Sergiy Byelozyorov wrote: > > What about DRY_RUN? > > wasn't supported before, so whatever. > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py > File tests/git_cl_test.py (right): > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... > tests/git_cl_test.py:1316: self.assertEqual(0, git_cl.main(['set-commit'])) > On 2016/04/13 17:15:03, Sergiy Byelozyorov wrote: > > also need a test for dry-run here > > see above - don't care about dry-run. > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... > tests/git_cl_test.py:1331: self.assertEqual(0, git_cl.main(['set-commit', > '-d'])) > On 2016/04/13 17:15:03, Sergiy Byelozyorov wrote: > > do you test non-dry-run scenario? > > no, it's same codepath. yes, it will provide line coverage, but no feature coverage... imho it's important to test set-commit feature without dry-run as well also landing a CL without leaving me a chance to read your replies and disagree make me wonder if you care about my comments at all - then why add me as a reviewer?
Message was sent while issue was closed.
On 2016/04/13 17:22:12, Sergiy Byelozyorov wrote: > On 2016/04/13 17:17:16, tandrii(chromium) wrote: > > thanks for review. replies below. > > > > https://codereview.chromium.org/1889483002/diff/20001/git_cl.py > > File git_cl.py (right): > > > > https://codereview.chromium.org/1889483002/diff/20001/git_cl.py#newcode1737 > > git_cl.py:1737: raise NotImplementedError() > > On 2016/04/13 17:15:02, Sergiy Byelozyorov wrote: > > > What about DRY_RUN? > > > > wasn't supported before, so whatever. > > > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py > > File tests/git_cl_test.py (right): > > > > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... > > tests/git_cl_test.py:1316: self.assertEqual(0, git_cl.main(['set-commit'])) > > On 2016/04/13 17:15:03, Sergiy Byelozyorov wrote: > > > also need a test for dry-run here > > > > see above - don't care about dry-run. > > > > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... > > tests/git_cl_test.py:1331: self.assertEqual(0, git_cl.main(['set-commit', > > '-d'])) > > On 2016/04/13 17:15:03, Sergiy Byelozyorov wrote: > > > do you test non-dry-run scenario? > > > > no, it's same codepath. > > yes, it will provide line coverage, but no feature coverage... imho it's > important to test set-commit feature without dry-run as well > also landing a CL without leaving me a chance to read your replies and disagree > make me wonder if you care about my comments at all - then why add me as a > reviewer? ah... sorry... CL got landed before I posted my comments
Message was sent while issue was closed.
On 2016/04/13 17:22:12, Sergiy Byelozyorov wrote: > On 2016/04/13 17:17:16, tandrii(chromium) wrote: > > thanks for review. replies below. > > > > https://codereview.chromium.org/1889483002/diff/20001/git_cl.py > > File git_cl.py (right): > > > > https://codereview.chromium.org/1889483002/diff/20001/git_cl.py#newcode1737 > > git_cl.py:1737: raise NotImplementedError() > > On 2016/04/13 17:15:02, Sergiy Byelozyorov wrote: > > > What about DRY_RUN? > > > > wasn't supported before, so whatever. > > > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py > > File tests/git_cl_test.py (right): > > > > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... > > tests/git_cl_test.py:1316: self.assertEqual(0, git_cl.main(['set-commit'])) > > On 2016/04/13 17:15:03, Sergiy Byelozyorov wrote: > > > also need a test for dry-run here > > > > see above - don't care about dry-run. > > > > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... > > tests/git_cl_test.py:1331: self.assertEqual(0, git_cl.main(['set-commit', > > '-d'])) > > On 2016/04/13 17:15:03, Sergiy Byelozyorov wrote: > > > do you test non-dry-run scenario? > > > > no, it's same codepath. > > yes, it will provide line coverage, but no feature coverage... imho it's > important to test set-commit feature without dry-run as well > also landing a CL without leaving me a chance to read your replies and disagree > make me wonder if you care about my comments at all - then why add me as a > reviewer? I understand what you are writing and I agree in general. And that's what I've been following along, but given huge untested space, i have to prioritize, and I believe this one is P2, just like implementing dry-run for Rietveld.
Message was sent while issue was closed.
On 2016/04/13 17:36:28, tandrii(chromium) wrote: > On 2016/04/13 17:22:12, Sergiy Byelozyorov wrote: > > On 2016/04/13 17:17:16, tandrii(chromium) wrote: > > > thanks for review. replies below. > > > > > > https://codereview.chromium.org/1889483002/diff/20001/git_cl.py > > > File git_cl.py (right): > > > > > > https://codereview.chromium.org/1889483002/diff/20001/git_cl.py#newcode1737 > > > git_cl.py:1737: raise NotImplementedError() > > > On 2016/04/13 17:15:02, Sergiy Byelozyorov wrote: > > > > What about DRY_RUN? > > > > > > wasn't supported before, so whatever. > > > > > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py > > > File tests/git_cl_test.py (right): > > > > > > > > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... > > > tests/git_cl_test.py:1316: self.assertEqual(0, git_cl.main(['set-commit'])) > > > On 2016/04/13 17:15:03, Sergiy Byelozyorov wrote: > > > > also need a test for dry-run here > > > > > > see above - don't care about dry-run. > > > > > > > > > https://codereview.chromium.org/1889483002/diff/20001/tests/git_cl_test.py#ne... > > > tests/git_cl_test.py:1331: self.assertEqual(0, git_cl.main(['set-commit', > > > '-d'])) > > > On 2016/04/13 17:15:03, Sergiy Byelozyorov wrote: > > > > do you test non-dry-run scenario? > > > > > > no, it's same codepath. > > > > yes, it will provide line coverage, but no feature coverage... imho it's > > important to test set-commit feature without dry-run as well > > also landing a CL without leaving me a chance to read your replies and > disagree > > make me wonder if you care about my comments at all - then why add me as a > > reviewer? > > I understand what you are writing and I agree in general. > And that's what I've been following along, but given huge untested space, i have > to prioritize, and I believe this one is P2, just like implementing dry-run for > Rietveld. LGTM, but please add a TODO.
Message was sent while issue was closed.
Message was sent while issue was closed.
https://codereview.chromium.org/1885963002 |
