New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 737720 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----



Sign in to add a comment

Review and cull keywords in tests change CL selection

Project Member Reported by seanmccullough@chromium.org, Jun 28 2017

Issue description

During 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.

 
Labels: Milestone-Worfklow
Cc: jparent@chromium.org qyears...@chromium.org
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.

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.

Comment 4 Deleted

[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.
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)?
Summary: Review and cull keywords in tests change CL selection (was: Review and cull keywords in tests change CL selection)
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.
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Owner: seanmccullough@chromium.org
Status: Fixed (was: Available)

Sign in to add a comment