Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2075993002:
Change remaining NeedsManualRebaseline to Failure or Pass Failure (Closed)
DescriptionChange remaining NeedsManualRebaseline to Failure or Pass Failure
Some tests have been marked NeedsManualRebaseline for a long
time without being rebaselined.
Some tests can't be rebaselined because of flakiness.
BUG=487344, 620126, 592409, 603997, 597221, 569139
Committed: https://crrev.com/d1997c29e5846c79df3efd9f383ce340f03f8efb
Cr-Commit-Position: refs/heads/master@{#400492}
Patch Set 1 #
Messages
Total messages: 15 (6 generated)
Description was changed from ========== Change remaining NeedsManualRebaseline to Failure or Pass Failure Some tests have been marked NeedsManualRebaseline for a long time. Some tests can't be rebaselined because of flakiness. BUG=87344,620126,592409,603997,597221,569139 ========== to ========== Change remaining NeedsManualRebaseline to Failure or Pass Failure Some tests have been marked NeedsManualRebaseline for a long time. Some tests can't be rebaselined because of flakiness. BUG=487344,620126,592409,603997,597221,569139 ==========
wangxianzhu@chromium.org changed reviewers: + dpranke@chromium.org, qyearsley@chromium.org
This CL also raises a question: Should we just disallow NeedsManualRebaseline?
On 2016/06/17 at 06:41:42, wangxianzhu wrote: > This CL also raises a question: Should we just disallow NeedsManualRebaseline? What has NeedsManualRebaseline been used for in the past? My idea of what it meant was "the current checked in baseline is incorrect, the actual results are correct, and auto-rebaseline is unable to fetch the correct baselines". Is that right? If there are still some cases where we can't automatically rebaseline, then maybe there's something to fix about the rebaseline tool?
On 2016/06/17 16:25:33, qyearsley wrote: > On 2016/06/17 at 06:41:42, wangxianzhu wrote: > > This CL also raises a question: Should we just disallow NeedsManualRebaseline? > > What has NeedsManualRebaseline been used for in the past? > > My idea of what it meant was "the current checked in baseline is incorrect, the > actual results are correct, and auto-rebaseline is unable to fetch the correct > baselines". Is that right? If there are still some cases where we can't > automatically rebaseline, then maybe there's something to fix about the > rebaseline tool? What it means is "We think we've fixed the bug and need new baselines, but we will need to stare at the new baselines to be sure, so don't just silently update them." It's *not* supposed to mean "leave these lines in for an indefinitely long period of time", and if people are doing that then the feature may be causing more harm than good. However, I wouldn't worry about it at the moment; I'd rather we spend our time working on try-side rebaselining.
On 2016/06/17 16:25:33, qyearsley wrote: > On 2016/06/17 at 06:41:42, wangxianzhu wrote: > > This CL also raises a question: Should we just disallow NeedsManualRebaseline? > > What has NeedsManualRebaseline been used for in the past? > > My idea of what it meant was "the current checked in baseline is incorrect, the > actual results are correct, and auto-rebaseline is unable to fetch the correct > baselines". Is that right? If there are still some cases where we can't > automatically rebaseline, then maybe there's something to fix about the > rebaseline tool? Based on my experience, I used NeedsManualRebaseline when the test had already other expectations, e.g. it was flaky or failing, while the CL did change its correct expectations. However, in such cases, except for simple test cases, we can hardly know what the correct expectations should be, especially there are multiple platform baselines. In many cases, I saw people add NeedsManualRebaselines then just forget them without actually rebaselining them. Another use case is during third party module roll. When a new version of third party module will change the output of some tests, the gardener mark these tests NeedsManualRebaseline so that the tests won't block the roll. After roll the gardener should rebaseline the tests (but they often forget). It seems to me failure expectations are sufficient for the cases. (BTW, which is orthogonal, to remind owners about failing tests, I think we should prohibit closing bugs with failure expectations. Perhaps we can run a cron job to check all bugs referenced from TestExpectations with failure expectations, and reopen closed bugs. Failures that won't be fixed should be in NeverFixTests)
Description was changed from ========== Change remaining NeedsManualRebaseline to Failure or Pass Failure Some tests have been marked NeedsManualRebaseline for a long time. Some tests can't be rebaselined because of flakiness. BUG=487344,620126,592409,603997,597221,569139 ========== to ========== Change remaining NeedsManualRebaseline to Failure or Pass Failure Some tests have been marked NeedsManualRebaseline for a long time without being rebaselined. Some tests can't be rebaselined because of flakiness. BUG=487344,620126,592409,603997,597221,569139 ==========
Filed crbug.com/621126 for long-standing NeedsManualRebaselines. What about this CL which changes long-standing NeedsManualRebaselines to Failure, and NeedsManualRebaseline for flaky tests to Pass Failure?
lgtm.
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2075993002/1
Message was sent while issue was closed.
Description was changed from ========== Change remaining NeedsManualRebaseline to Failure or Pass Failure Some tests have been marked NeedsManualRebaseline for a long time without being rebaselined. Some tests can't be rebaselined because of flakiness. BUG=487344,620126,592409,603997,597221,569139 ========== to ========== Change remaining NeedsManualRebaseline to Failure or Pass Failure Some tests have been marked NeedsManualRebaseline for a long time without being rebaselined. Some tests can't be rebaselined because of flakiness. BUG=487344,620126,592409,603997,597221,569139 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Change remaining NeedsManualRebaseline to Failure or Pass Failure Some tests have been marked NeedsManualRebaseline for a long time without being rebaselined. Some tests can't be rebaselined because of flakiness. BUG=487344,620126,592409,603997,597221,569139 ========== to ========== Change remaining NeedsManualRebaseline to Failure or Pass Failure Some tests have been marked NeedsManualRebaseline for a long time without being rebaselined. Some tests can't be rebaselined because of flakiness. BUG=487344,620126,592409,603997,597221,569139 Committed: https://crrev.com/d1997c29e5846c79df3efd9f383ce340f03f8efb Cr-Commit-Position: refs/heads/master@{#400492} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/d1997c29e5846c79df3efd9f383ce340f03f8efb Cr-Commit-Position: refs/heads/master@{#400492} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
