Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2392173002:
Simplify and add comments to copy-existing-baselines-internal test. (Closed)
DescriptionSimplify and add comments to copy-existing-baselines-internal test.
The purpose of this CL is to improve the readability of this test, to
try to better indicate what webkit-patch copy-existing-baselines-internal
does.
Committed: https://crrev.com/b05c1ec314271a089737782a313251634cf2e885
Cr-Commit-Position: refs/heads/master@{#423747}
Patch Set 1 #Patch Set 2 : Simplify and add comments to copy-existing-baselines-internal test. #
Total comments: 5
Patch Set 3 : Don't use OutputCapture in TestCopyExistingBaselinesInternal. #Messages
Total messages: 15 (7 generated)
qyearsley@chromium.org changed reviewers: + dpranke@chromium.org, wkorman@chromium.org
Since the main purpose of this CL is to try to make the test more understandable, the main thing to review is if I've added anything that's confusing or doesn't make sense :-)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I defer this to wkorman@ :) Let me know if you don't think you can review this adequately.
lgtm https://codereview.chromium.org/2392173002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py (left): https://codereview.chromium.org/2392173002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py:194: finally: Just to make sure I understand the change, are we removing try/finally because they shouldn't fail in general and if they do we'd prefer the test fail and inform us re: what happened? https://codereview.chromium.org/2392173002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py (right): https://codereview.chromium.org/2392173002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py:142: # The idea is, if in the next step wae download new baselines for Mac10.11 wae -> we
https://codereview.chromium.org/2392173002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py (left): https://codereview.chromium.org/2392173002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py:194: finally: On 2016/10/06 at 21:22:29, wkorman wrote: > Just to make sure I understand the change, are we removing try/finally because they shouldn't fail in general and if they do we'd prefer the test fail and inform us re: what happened? Right -- as far as I understand, if any sort of exception is raised here we want to fail the test, which is what I think should happen. I think that the try/finally blocks might be here just because that was used elsewhere in unit tests that use OutputCapture to assert things about the output (or hide/ignore the output), but here it's unnecessary. Actually, in this case, I think that asserting things about the output may be beside the point of the test (and after http://crrev.com/2397573002, this shouldn't be outputting SCM changes anyway), and hiding the output is unnecessary because that's done by the test runner. So, in the latest patchset, I've removed use of OutputCapture from this test case; does that seem OK? https://codereview.chromium.org/2392173002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py (right): https://codereview.chromium.org/2392173002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py:142: # The idea is, if in the next step wae download new baselines for Mac10.11 On 2016/10/06 at 21:22:29, wkorman wrote: > wae -> we Done
lgtm https://codereview.chromium.org/2392173002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py (left): https://codereview.chromium.org/2392173002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py:194: finally: On 2016/10/06 22:00:29, qyearsley wrote: > On 2016/10/06 at 21:22:29, wkorman wrote: > > Just to make sure I understand the change, are we removing try/finally because > they shouldn't fail in general and if they do we'd prefer the test fail and > inform us re: what happened? > > Right -- as far as I understand, if any sort of exception is raised here we want > to fail the test, which is what I think should happen. > > I think that the try/finally blocks might be here just because that was used > elsewhere in unit tests that use OutputCapture to assert things about the output > (or hide/ignore the output), but here it's unnecessary. > > Actually, in this case, I think that asserting things about the output may be > beside the point of the test (and after http://crrev.com/2397573002, this > shouldn't be outputting SCM changes anyway), and hiding the output is > unnecessary because that's done by the test runner. > > So, in the latest patchset, I've removed use of OutputCapture from this test > case; does that seem OK? Yes, thanks.
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...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Simplify and add comments to copy-existing-baselines-internal test. The purpose of this CL is to improve the readability of this test, to try to better indicate what webkit-patch copy-existing-baselines-internal does. ========== to ========== Simplify and add comments to copy-existing-baselines-internal test. The purpose of this CL is to improve the readability of this test, to try to better indicate what webkit-patch copy-existing-baselines-internal does. Committed: https://crrev.com/b05c1ec314271a089737782a313251634cf2e885 Cr-Commit-Position: refs/heads/master@{#423747} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b05c1ec314271a089737782a313251634cf2e885 Cr-Commit-Position: refs/heads/master@{#423747} |
