Bug 61888 - nrwt: should skip chromium test expectation overrides on upstream bots
Summary: nrwt: should skip chromium test expectation overrides on upstream bots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-01 15:49 PDT by Dirk Pranke
Modified: 2011-07-29 14:05 PDT (History)
4 users (show)

See Also:


Attachments
chromium-specific version (3.26 KB, patch)
2011-06-01 15:55 PDT, Dirk Pranke
abarth: review+
dpranke: commit-queue?
Details | Formatted Diff | Diff
generic version (5.58 KB, patch)
2011-06-01 16:08 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-06-01 15:49:43 PDT
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.
Comment 1 Dirk Pranke 2011-06-01 15:55:18 PDT
Created attachment 95680 [details]
chromium-specific version
Comment 2 Dirk Pranke 2011-06-01 16:08:04 PDT
Created attachment 95683 [details]
generic version
Comment 3 Dirk Pranke 2011-06-01 16:08:50 PDT
actually, now that I've cleaned up the generic version a bit, it's kind of a toss-up.
Comment 4 Ojan Vafai 2011-06-01 20:17:35 PDT
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.
Comment 5 Stephen White 2011-06-02 06:27:10 PDT
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.
Comment 6 Eric Seidel (no email) 2011-06-02 08:34:56 PDT
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).
Comment 7 Adam Barth 2011-06-02 09:45:12 PDT
You just need to co-ordinated with the webkit gardener when rolling ski a (which you'll need to do anyway without commit bit).
Comment 8 Tony Chang 2011-06-02 10:54:17 PDT
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?).
Comment 9 Dirk Pranke 2011-06-02 10:55:48 PDT
(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.
Comment 10 Dirk Pranke 2011-06-02 10:57:54 PDT
(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.
Comment 11 Dirk Pranke 2011-06-03 12:04:06 PDT
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.
Comment 12 Adam Barth 2011-06-03 15:38:12 PDT
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.
Comment 13 Dirk Pranke 2011-06-03 15:49:55 PDT
(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.
Comment 14 Dirk Pranke 2011-06-03 16:02:09 PDT
Committed r88073: <http://trac.webkit.org/changeset/88073>
Comment 15 Stephen White 2011-07-28 12:21:41 PDT
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.
Comment 16 Adam Barth 2011-07-28 12:54:56 PDT
Perhaps skia should roll into WebKit before it rolls into Chromium?
Comment 17 Stephen White 2011-07-28 13:05:41 PDT
(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.
Comment 18 Adam Barth 2011-07-29 10:35:22 PDT
> 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?
Comment 19 Stephen White 2011-07-29 11:42:20 PDT
(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.
Comment 20 Adam Barth 2011-07-29 11:47:46 PDT
> 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.
Comment 21 Dirk Pranke 2011-07-29 13:20:00 PDT
(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.
Comment 22 Adam Barth 2011-07-29 13:28:27 PDT
> 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)
Comment 23 Stephen White 2011-07-29 13:40:08 PDT
(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.
Comment 24 Adam Barth 2011-07-29 13:42:19 PDT
/me will write a patch.
Comment 25 Adam Barth 2011-07-29 14:05:51 PDT
https://bugs.webkit.org/show_bug.cgi?id=65390