Review and cull keywords in tests change CL selection |
|||||
Issue descriptionDuring the ChOps project review, dpranke said there were some keywords to cull from this dialog: https://screenshot.googleplex.com/JOPCHvGzjSc Which ones? I understand there are some mutually exclusive options, some which obviate others, but are there any that should not be there at all? Reassign to me once we have this list.
,
Jun 29 2017
We don't need to support "Missing", "Rebaseline", ""Linux32", "Precise", or "Trusty". The first two should never happen; the last three are no longer needed. I think we also shouldn't offer the option of "NeedsRebaseline", or "NeedsManualRebaseline"; they are legal, but (a) "NeedsRebaseline" requires rebaseline-o-matic to be running, and we want to turn that down, and (b) a sheriff should just revert a change, rather than letting something broken stay in the tree. With the functionality we have in rebaseline-cl, there's not really a good excuse for broken baselines to stay in the tree. Or, at least that's my thinking. qyearsley@, what do you think? One could also imagine a more drastic form of this that lets sheriffs have much less control. For example, one could argue that all a sheriff should really do is mark a test as "flaky" (for some definition of flaky), generically. It would then become the developer's problem to refine it that to a more tightly-constrained specification. Partially this is because I don't want sheriffs to have to think about this too much (it shouldn't be their problem) and partially I think we just give people too much power to constrain where tests are flaky or failing, and I think that actually leads them to ignore failures and flakes more. But, that's a perhaps more radical and controversial proposal.
,
Jun 29 2017
I agree! First, looking more specifically at what can be done next for each keyword: - "Precise" and "Linux32" already don't exist and should never occur. - "Missing" and "Trusty" can be removed any time. - "Rebaseline" should be removed if/when we remove "webkit-patch rebaseline-expectations". - "NeedsRebaseline" should be removed at the same time as the auto-rebaseline code, very soon. - "NeedsManualRebaseline" could also be removed, assuming its use-case can be covered by [ Pass Failure ]. There's also "WontFix", which means the same thing as "Skip"; that could also be removed. Regarding the idea of replacing all of the specific flaky expectations with just one "Flaky" keyword -- I personally think this sounds OK, because the specific way that the test is flaky should always be described in the associated bug, so we shouldn't be losing information. I would expect the the "Flaky" keyword to just mean "keep running this but ignore the results" i.e. basically what "NeedsManualRebaseline" does now.
,
Jun 29 2017
[Revising comment 4 because I had an incomplete sentence.] Addendum: "Missing" is also a possible "actual" result, it's just not an acceptable "expected" result. (Kind of like how "Text" is an actual result but not expected.) Maybe we actually want to keep this keyword as a possible actual result? We could also potentially totally remove it, and merge Text, Image, Text+Image, Audio and Missing into the one actual result "Failure". This is bug 654500 . "Trusty" is the specific version specifier that's listed in https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/config/builders.py, and it's the only supported version for Linux -- just like how "KitKat" is the only supported version of "Android". Keeping the idea that one specifier string corresponds to one full port name (port + version) might make it easier to add another supported version if we want to do that. But regardless of this, we can still disallow it in TestExpectations. Also: bug 735176 is a subset of this bug, only talk about the rebaseline expectations.
,
Jun 29 2017
qyearsley: Re [Needs[Manual]]Rebaseline - deprecation pending removal of "webkit-patch rebaseline-expectations": is there a bug tracking this that I can reference in a TODO (and block this issue on)?
,
Jun 29 2017
Just filed bug 738152 for that. Note that currently "Rebaseline" lines should never be checked in, so tools that read and commit to TestExpectations should never consider this a valid expectation anyway, currently.
,
Jun 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7cd2d63b3831dc53449c8585f1e84b946c02e61a commit 7cd2d63b3831dc53449c8585f1e84b946c02e61a Author: Quinten Yearsley <qyearsley@google.com> Date: Thu Jun 29 21:39:36 2017 Don't accept "Trusty" in TestExpectations. After this change, "Trusty" is still the specifier string for the "linux-trusty" port, just as "KitKat" is the specifier for the "android-kitkat" port; but since we currently support only one version of linux, we can mark Trusty as not recognized in TestExpectations, just as KitKat is not recognized. Bug: 737720 Change-Id: I13608b695ecf09d4153ffbe27b26e2cf19c09ba2 Reviewed-on: https://chromium-review.googlesource.com/556300 Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Quinten Yearsley <qyearsley@chromium.org> Cr-Commit-Position: refs/heads/master@{#483496} [modify] https://crrev.com/7cd2d63b3831dc53449c8585f1e84b946c02e61a/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py [modify] https://crrev.com/7cd2d63b3831dc53449c8585f1e84b946c02e61a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_expectations_updater_unittest.py
,
Jun 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/infra/infra/+/835794d3b750cc5e7d4f704747f0f92084c67c76 commit 835794d3b750cc5e7d4f704747f0f92084c67c76 Author: Sean McCullough <seanmccullough@chromium.org> Date: Fri Jun 30 00:37:57 2017 [som] Remove deprecated layout test expectation values Bug: 737720 Change-Id: Ied788d12edcdf54ca373c1b69e880ae03ae08b7d Reviewed-on: https://chromium-review.googlesource.com/556463 Commit-Queue: Sean McCullough <seanmccullough@chromium.org> Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> [modify] https://crrev.com/835794d3b750cc5e7d4f704747f0f92084c67c76/go/src/infra/appengine/sheriff-o-matic/frontend/elements/som-test-expectations/som-edit-expectation-form.js
,
Jul 11 2017
,
Jul 24 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by seanmccullough@chromium.org
, Jun 28 2017