Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412533002:
Make Rebaseline/NeedsRebaseline/NeedsManualRebaseline not conflict with Pass lines. (Closed)
DescriptionMake Rebaseline/NeedsRebaseline/NeedsManualRebaseline not conflict with Pass lines.
As per https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OI_gXMX7yUs.
Committed: https://crrev.com/30fe1b01a50cefa35a0578311944e3b09cb27f38
Cr-Commit-Position: refs/heads/master@{#357741}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Fix tests as per code review #
Messages
Total messages: 13 (2 generated)
ojan@chromium.org changed reviewers: + dpranke@chromium.org, eae@chromium.org, joelo@chromium.org, pdr@chromium.org, wangxianzhu@chromium.org
yeesh, I'm gonna have to actually think about this one :). I'll try to get to it tomorrow (most likely, possibly later tonight).
No worries. This one took surprisingly long to get right. On Thu, Oct 15, 2015, 6:33 PM <dpranke@chromium.org> wrote: > yeesh, I'm gonna have to actually think about this one :). I'll try to get > to it > tomorrow (most likely, possibly later tonight). > > https://codereview.chromium.org/1412533002/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
No worries. This one took surprisingly long to get right. On Thu, Oct 15, 2015, 6:33 PM <dpranke@chromium.org> wrote: > yeesh, I'm gonna have to actually think about this one :). I'll try to get > to it > tomorrow (most likely, possibly later tonight). > > https://codereview.chromium.org/1412533002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py (right): https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:747: if PASS in expectation_line.parsed_expectations and self._expects_rebaseline(prev_expectation_line): two questions to start with ... 1) Do we want to allow a test with [ Pass Failure ] to be rebaselined? 2) Don't we also need to look at the set of matching configurations? I think the code as written would say that [ Mac Failure ] does not conflict with [ Rebaseline ] https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py (right): https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:470: Bug(exp) failures/expected/text.html [ NeedsRebaseline ]""", is_lint_mode=True) why does this raise an error? (by which I mean, both, why do you think this should raise an error, which is kinda the same question as the other comment, and what in the code will cause an error to be raised)? https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:483: def test_duplicates_rebaseline_no_conlict(self): typo: "conflict" https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:488: Bug(exp) failures/expected/crash.html [ Pass Failure ] It's not completely clear to me that this should be allowed ... https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:497: """, is_lint_mode=False) it's a bit confusing that these three things change both the filename and the keywords. I suppose you change the filenames so that you can do this all in a single call to parse_exp()? Maybe it would be better to do multiple calls? https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:499: self.assert_exp_list('failures/expected/crash.html', [PASS, FAIL, REBASELINE]) why isn't this [ PASS, CRASH, REBASELINE ] ? https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:503: def test_duplicates_rebaseline_no_conlict_linting(self): same typo. https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:516: """, is_lint_mode=True) I'm not clear on why you have both this and the previous test case. Are you simply testing that no assertions were raised in the parse?
Sorry it took me so long to respond on this. https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py (right): https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:747: if PASS in expectation_line.parsed_expectations and self._expects_rebaseline(prev_expectation_line): On 2015/10/17 at 00:12:29, Dirk Pranke wrote: > two questions to start with ... > > 1) Do we want to allow a test with [ Pass Failure ] to be rebaselined? Yes. > 2) Don't we also need to look at the set of matching configurations? I think the code as written would say that [ Mac Failure ] does not conflict with [ Rebaseline ] I don't think so. The first part of the if statement here prevents that. https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py (right): https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:470: Bug(exp) failures/expected/text.html [ NeedsRebaseline ]""", is_lint_mode=True) On 2015/10/17 at 00:12:29, Dirk Pranke wrote: > why does this raise an error? (by which I mean, both, why do you think this should raise an error, which is kinda the same question as the other comment, and what in the code will cause an error to be raised)? Yup. See previous comment. https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:488: Bug(exp) failures/expected/crash.html [ Pass Failure ] On 2015/10/17 at 00:12:29, Dirk Pranke wrote: > It's not completely clear to me that this should be allowed ... Yeah, I waffled on this a bit too. In the end, the thing that swayed my judgement is that ideally the rebaseline bot would do exactly the same thing as a local "webkit-patch rebaseline-expectations" with Rebaseline lines. It keeps NeedsRebaseline and Rebaseline working the same way. https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:497: """, is_lint_mode=False) On 2015/10/17 at 00:12:29, Dirk Pranke wrote: > it's a bit confusing that these three things change both the filename and the keywords. > > I suppose you change the filenames so that you can do this all in a single call to parse_exp()? > Maybe it would be better to do multiple calls? Yup. sgtm. https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:499: self.assert_exp_list('failures/expected/crash.html', [PASS, FAIL, REBASELINE]) On 2015/10/17 at 00:12:29, Dirk Pranke wrote: > why isn't this [ PASS, CRASH, REBASELINE ] ? Changed these all to just use text.html. https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:516: """, is_lint_mode=True) On 2015/10/17 at 00:12:29, Dirk Pranke wrote: > I'm not clear on why you have both this and the previous test case. Are you simply testing that no assertions were raised in the parse? It's testing is_lint_mode=True and is_lint_mode=False since they have different code paths in terms of merging expectations lines, the former always merges all lines, the latter only does so if they have rebaseline expectations or are from different files.
lgtm https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py (right): https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:747: if PASS in expectation_line.parsed_expectations and self._expects_rebaseline(prev_expectation_line): On 2015/11/03 16:25:39, ojan wrote: > > 2) Don't we also need to look at the set of matching configurations? I think > the code as written would say that [ Mac Failure ] does not conflict with [ > Rebaseline ] > > I don't think so. The first part of the if statement here prevents that. Ok, you're probably right, looking at it again. I'm not sure what I was thinking before. https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py (right): https://codereview.chromium.org/1412533002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:470: Bug(exp) failures/expected/text.html [ NeedsRebaseline ]""", is_lint_mode=True) ok.
The CQ bit was checked by ojan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412533002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412533002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/30fe1b01a50cefa35a0578311944e3b09cb27f38 Cr-Commit-Position: refs/heads/master@{#357741}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1550703002/ by wkorman@chromium.org. The reason for reverting is: Interim fix for http://crbug.com/569175 until we can update script to not strip Pass/Failure lines for tests also matching NeedsRebaseline.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
