Bug 61888 – nrwt: should skip chromium test expectation overrides on upstream bots

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

If there are test_expectations checked into a downstream Chromium checkout, they are automatically picked up by both the canaries and the DEPS bots. There is therefore no way to test whether the upstream test_expectations.txt themselves are sufficient to keep the tree green (which one might want to do prior to doing a webkit roll).

So, we've decided to modify the code so that the overrides are ignored if the builder_name is set and does not include "(deps)" in the name.

A downside of this change is that a chromium-only committer will no longer be able to fix a failure on the canaries by just inserting a downstream expectation. Hopefully this will not be much of an issue in practice.


Note that there are two ways to implement this change. We could either make the code generically aware of the significance of "(deps)" and skip the overrides generically, or we could push all of this logic inside the chromium code.

There does not appear to be a way to write tests for this that will cover both cases :(

I will post patches that demonstrate both approaches, but the chromium-only approach seems slightly simpler for now.

[Dirk Pranke]

Created attachment 95680 [details]
chromium-specific version

[Dirk Pranke]

Created attachment 95683 [details]
generic version

[Dirk Pranke]

actually, now that I've cleaned up the generic version a bit, it's kind of a toss-up.

[Ojan Vafai]

Meh. Either solution seems fine to me. The chromium-only approach appeals to me in that it's just fewer modified lines/files.

I'm not sure DUMMY_BUILDER_NAME needs to use overrides though. It would somewhat simplify that if-check.

[Stephen White]

If I understand this correctly, there may be a problem:  since Skia rolls into chromium first, if a Skia roll introduces test failures (which are added to downstream test_expectations), there will be no way to get the canaries green without upstreaming the test_expectations.  Some of the skia gardeners are not WebKit committers, so they won't be able to do this without going through webkit's bugzilla, commit-queue, etc.

I'm not sure this is a blocking issue (since it'll only affect the WebKit gardener, and only until the expectations are upstreamed), but I thought I'd mention it.

[Eric Seidel (no email)]

Comment on attachment 95680 [details]
chromium-specific version

Cleared review? from obsolete attachment 95680 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Adam Barth]

You just need to co-ordinated with the webkit gardener when rolling ski a (which you'll need to do anyway without commit bit).

[Tony Chang]

I feel the pain for you guys who are trying to improve skia.  There's pain on so many sides :(

I've also mentioned to evan that it would be possible to roll skia in WebKit first (in Source/WebKit/chromium/DEPS) before rolling it in chromium, but then you have to remember to update two DEPS values when rolling webkit.  We could also resurrect From() so that chromium gets the version of skia from WebKit, but then rolling skia would be blocked on webkit rolls.

Sorry for derailing this bug.  It might be worth brainstorming over email how to make this less painful (maybe we need a ToT chromium, ToT webkit, ToT skia builder-- that would at least tell you which tests to add to test_expectations.txt in advance?).

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 95680 [details])
> Cleared review? from obsolete attachment 95680 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

Actually, I did want that version reviewed, since I think there's a slight agreement that the chromium-only version might be better. Re-setting the flag.

[Dirk Pranke]

(In reply to comment #4)
> Meh. Either solution seems fine to me. The chromium-only approach appeals to me in that it's just fewer modified lines/files.
> 
> I'm not sure DUMMY_BUILDER_NAME needs to use overrides though. It would somewhat simplify that if-check.

The reason the DUMMY_BUILDER_NAME variant uses the overrides is so --lint-test-files will pick up and lint the overrides as well as the upstream version; there isn't a good alternative AFAICT, and it seemed important to make sure both files were linted. I also couldn't think of a way that the downstream version would mask any upstream-only lint errors, so it seemed safer.

[Dirk Pranke]

One additional thought is that we could create *two* downstream expectations files, one that only applied to the DEPS bots and one that only applied to the canaries. That way a skia change that broke the canaries could still be fixed by someone with chromium-only committing rights.

That said, I'd be inclined to not do this until it was clear that this was a problem.

[Adam Barth]

Comment on attachment 95680 [details]
chromium-specific version

View in context: https://bugs.webkit.org/attachment.cgi?id=95680&action=review

I think we should move forward with this approach.  Introducing a yet another test_expectations file is too much complexity.  We'll find a way to do Skia rolls without this functionality.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:265
> +        # See http://bugs.webkit.org/show_bug.cgi?id=61888 for the rationale for this.

I'd remove this comment.  Folks can use svn blame to find the bug number.

[Dirk Pranke]

(In reply to comment #12)
> (From update of attachment 95680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95680&action=review
> 
> I think we should move forward with this approach.  Introducing a yet another test_expectations file is too much complexity.  We'll find a way to do Skia rolls without this functionality.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:265
> > +        # See http://bugs.webkit.org/show_bug.cgi?id=61888 for the rationale for this.
> 
> I'd remove this comment.  Folks can use svn blame to find the bug number.

Okay.

[Dirk Pranke]

Committed r88073: <http://trac.webkit.org/changeset/88073>

[Stephen White]

Update:  I think we need to reconsider this change.  This is making skia rolls very painful.

When we roll skia, in order to prevent the WebKit canaries going red, we have to land an upstream WebKit change to pre-emptively suppress the failures.  Then we land the skia roll.  However, if this is done before the WebKit roll brings the suppressions into Chrome, we have to *also* suppress the failures in downstream test_expectations.  Then rebaseline upstream, and remove the expectations in both places.  So up to five patches.

The amount of pain this is causing the skia gardener is greater than the amount of good it is causing the WebKit gardener, IMHO.

Also, it seems that this change also inadvertently affected the layout test trybots:  since they don't have "(deps)" in their name, they also ignore the downstream test_expectations, leading to unexpected failures during try runs.

[Adam Barth]

Perhaps skia should roll into WebKit before it rolls into Chromium?

[Stephen White]

(In reply to comment #16)
> Perhaps skia should roll into WebKit before it rolls into Chromium?

Although we considered it, I don't think that's practical at this time.  There's a lot of work going on in Skia, and to add the lag of a WebKit roll to that work would be prohibitive.  As one datapoint, there are a number of merges going from skia to the release branch right now.  To add a WebKit merge to that process seems like it would be a nightmare.

Also, since we don't have full coverage of all tests on the WebKit bots, a skia change which introduced a ChromeOS or pyauto regression (or any one of the many bots we don't have WebKit canary coverage for) would require a rollback in skia, and a merge forward in WebKit to fix.

[Adam Barth]

> Although we considered it, I don't think that's practical at this time.  There's a lot of work going on in Skia, and to add the lag of a WebKit roll to that work would be prohibitive.  As one datapoint, there are a number of merges going from skia to the release branch right now.  To add a WebKit merge to that process seems like it would be a nightmare.

Whether we fix the skia revision in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS or http://src.chromium.org/viewvc/chrome/trunk/src/DEPS doesn't have any bearing on how you merge changes to release branches.

> Also, since we don't have full coverage of all tests on the WebKit bots, a skia change which introduced a ChromeOS or pyauto regression (or any one of the many bots we don't have WebKit canary coverage for) would require a rollback in skia, and a merge forward in WebKit to fix.

We have that problem for every WebKit change.  This bug probably isn't the best place to have this discussion.  Maybe we should move the discussion to chromium-dev?

[Stephen White]

(In reply to comment #18)
> > Although we considered it, I don't think that's practical at this time.  There's a lot of work going on in Skia, and to add the lag of a WebKit roll to that work would be prohibitive.  As one datapoint, there are a number of merges going from skia to the release branch right now.  To add a WebKit merge to that process seems like it would be a nightmare.
> 
> Whether we fix the skia revision in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS or http://src.chromium.org/viewvc/chrome/trunk/src/DEPS doesn't have any bearing on how you merge changes to release branches.

Yeah, I think I was wrong about that -- any merges to the skia Release branch won't require a WebKit merge, since the release branch WebKit DEPS will already be pointing at the Skia release branch (I think?)

> > Also, since we don't have full coverage of all tests on the WebKit bots, a skia change which introduced a ChromeOS or pyauto regression (or any one of the many bots we don't have WebKit canary coverage for) would require a rollback in skia, and a merge forward in WebKit to fix.
> 
> We have that problem for every WebKit change.

Right, this would just extend the problem to every skia change as well.

> This bug probably isn't the best place to have this discussion.  Maybe we should move the discussion to chromium-dev?

I started to write out the pros and cons of each option, and after considering the above I think merging skia->WebKit actually comes out ahead.  I'm going to run it by the skia folks again first to see what they think.

I do think that the issue of the trybots not respecting the downstream test_expectations should be fixed, though.

[Adam Barth]

> I do think that the issue of the trybots not respecting the downstream test_expectations should be fixed, though.

Definitely.  We should coordinate with maruel to make that happen.

Thanks.

[Dirk Pranke]

(In reply to comment #20)
> > I do think that the issue of the trybots not respecting the downstream test_expectations should be fixed, though.
> 
> Definitely.  We should coordinate with maruel to make that happen.
> 
> Thanks.

You can simply modify chromium.py to recognize the tryserver builder names and include the file. I don't think maruel needs to do anything.

[Adam Barth]

> You can simply modify chromium.py to recognize the tryserver builder names and include the file. I don't think maruel needs to do anything.

As these the names:

linux_layoutm mac_layout, win_layout ?

(from http://build.chromium.org/p/tryserver.chromium/waterfall)

[Stephen White]

(In reply to comment #22)
> > You can simply modify chromium.py to recognize the tryserver builder names and include the file. I don't think maruel needs to do anything.
> 
> As these the names:
> 
> linux_layoutm mac_layout, win_layout ?
> 
> (from http://build.chromium.org/p/tryserver.chromium/waterfall)

There are Release flavours of each as well:  linux_layout_rel, mac_layout_rel, win_layout_rel.

[Adam Barth]

/me will write a patch.

[Adam Barth]

https://bugs.webkit.org/show_bug.cgi?id=65390