Bug 86749 – add skia test_expectations override file to chromium NRWT

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Elliot Poger]

I plan to add another test_expectations override file to NRWT when running for a chromium-flavored port.

This new file will be similar to the expectations override file we already pull from the Chromium repo ( http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/test_expectations.txt ), but this one will be pulled from the Skia repo.  If the file is not found, then NRWT will just continue without it.

This work is to help with the issues discussed in https://goto.google.com/SkiaRebaseliningProcess (Googler-only link, sorry)

I'm working on the CL now, and will attach it for review here once it's done; in the meanwhile, feedback on the idea is welcome.

[Elliot Poger]

+rniwa , he might have something to say about this too

[Ryosuke Niwa]

(In reply to comment #0)
> This new file will be similar to the expectations override file we already pull from the Chromium repo ( http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/test_expectations.txt ), but this one will be pulled from the Skia repo.  If the file is not found, then NRWT will just continue without it.
> 
> This work is to help with the issues discussed in https://goto.google.com/SkiaRebaseliningProcess (Googler-only link, sorry)

Please post the relevant part of the discussion here so that people can see it.


Presumably, this expectation file is only used as a temporary place to put test expectations? Given that Skia folks were the only people who were interested in keeping the chromium version of test_expectation anyway, I'm inclined to say that we should just get rid of the test_expectation file in Chromium tree once this is implemented.

[Ryosuke Niwa]

I don't want to live in a world where I have 3 different test_expectations.txt each living in a separate repository. That's just a maintenance hell to me.

[Dirk Pranke]

Elliot, how long do you expect entries to live in the skia-specific file? I think if the lifetime is roughly equivalent to the lifetime of entries in the chromium overrides file (i.e., the file is usually empty and entries are upstreamed as soon as possible), then this seems reasonable.

Also, I agree that if we can get rid of the chromium overrides by switching to this that would be a good thing. I'm not sure how often we have downstream failures that are caused by things other than skia changes at this point.

[Ryosuke Niwa]

On my second thought, V8 folks also use the test_expectations.txt file in the chromium repository as well. So maybe this isn't such a great idea after all.

Also, WebKit gardeners have been trained to pay attention to the test_expectations.txt file in the chromium repository so I'm afraid moving it to the Skia repository will result in the file getting less attention, and entries not being removed from the file promptly.

[Mike Reed]

The skia team will be on the hook to remove entries from this file in a timely fashion, not the gardeners. This is meant to keep the bots green during a big rebaseline event, not to hide long-term bugs.

[Elliot Poger]

> Elliot, how long do you expect entries to live in the skia-specific file? I think if the lifetime is roughly equivalent to the lifetime of entries in the chromium overrides file (i.e., the file is usually empty and entries are upstreamed as soon as possible), then this seems reasonable.

The idea is that the skia test_expectations.txt file would typically be empty, and tests would never stay in there for more than a week or two (enough time for all the back-n-forth deps rolls to go through, and new baselines to be generated/checked in).

The idea is NOT for entries in this file to be "upstreamed" to the main webkit test_expectations file.  See more details below.

Here's the most relevant section of the March 9 discussion notes, as linked to within https://goto.google.com/SkiaRebaseliningProcess :

> Currently, overlapping test expectations rules aren’t allowed
> - Should we have a separate override file just for Skia?
> - There are some concerns about changing the lint rules for the test_expectations file
>  - Disambiguation: which one of these expectations is “right”?
>  - File will grow without bounds

As I recall, the general takeaway of that discussion was: "the Skia guys really will rebaseline their tests rather than leaving expected failures in place forever, so we should allow their test_expectations to be overlapping with existing ones and we should let the Skia team rebaseline those tests themselves".

Another way of putting it, with a concrete use case:
- We (Skia) have a fix for http://crbug.com/124881 ('Calendar rasters very slowly through tiled SkPicture playback path') that will require us to rebaseline ~900 webkit layout_tests.
- Adding those expectations to the main webkit test_expectations file is extremely painful, because overlapping rules are not allowed in there.
- We want to be able to simply disable large groups of layout_tests while we work on the rebaseline, keeping layout_test results green for everyone else.
- We don't want gardeners to rebaseline our tests; we want to look at the pixel diffs and verify that they are as expected.
- We don't want gardeners to move our expectations into the main webkit test_expectations file, because they will often overlap with existing rules and thus fail the lint check (even though they work just fine).

From our discussion, it seemed that a Skia-specific test_expectations file was the best way to accomplish those goals.

> Also, WebKit gardeners have been trained to pay attention to the test_expectations.txt file in the chromium repository so I'm afraid moving it to the Skia repository will result in the file getting less attention, and entries not being removed from the file promptly.

That's actually a good thing!  The problem we have had with the chromium test_expectations file is that gardeners always want to clean out chromium test_expectations, which requires the new expectations to be written into webkit test_expectations, which requires the new expectations to NOT overlap with any of the thousands of expectations already in that file.

> I'm inclined to say that we should just get rid of the test_expectation file in Chromium tree once this is implemented.

Getting rid of the chromium version of test_expectations.txt is fine with me (if we add the one in Skia); I don't care either way.

[Elliot Poger]

Created attachment 142531 [details]
Patch

[Elliot Poger]

Comment on attachment 142531 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:328
>          if builder_name != 'DUMMY_BUILDER_NAME' and not '(deps)' in builder_name and not builder_name in self.try_builder_names:

This patch handles the new skia test_expectations file exactly the same as the chromium test_expectations file (only is observed on certain builders, etc.)

Given the comments in https://bugs.webkit.org/show_bug.cgi?id=86749 , I think it actually makes more sense to handle the skia test_expectations file a bit differently... so I will be uploading a second patch to this same bug soon.  Then we can decide which approach is better...

So, no need to review this patch until the next one shows up.

[Elliot Poger]

Created attachment 142538 [details]
Patch

[Elliot Poger]

Comment on attachment 142538 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:345
> +        # Always add Skia test_expectations overrides, if they exist.

Patch #2 adds the Skia test_expectations file on *all* builders, while maintaining the current behavior of only adding the Chromium test_expectations file on *certain* builders.

The difference in behavior is because expectations are only supposed to live in the Chromium test_expectations file "on their way" to living in the main webkit test_expectations file, while the Skia test_expectations should NOT be migrated over by gardeners.

Ready for review.

[Ojan Vafai]

Comment on attachment 142538 [details]
Patch

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

IMO, this is a little gross, but if it makes doing Skia rolls considerably easier, then I think it's a worthwhile tradeoff.

Code looks fine to me except for the style nits, but I'd like dpranke to weigh do the final r+.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:323
> +    def get_expectations_file_contents(self, filetype, filepath):

Nit: webkit generally avoids putting "get" in method names. expectations_file_contents would be fine. A lot of the layout_tests code doesn't adhere to this since it came from the Chromium tree initially, but we're slowly moving it over to a more webkitty style.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:330
> +        """Return the contents of a test_expectations file, or '' if not found.
> +
> +        Args:
> +            filetype: which type of test_expectations override file (only for
> +                      logging purposes)
> +            filepath: path to the file
> +        """

Ditto: webkit is allergic to "what" comments. This comment states what the code does pretty clearly if you take a quick look at it. Comments in webkit should only be to explain why (e.g. why are we doing some crazy, non-obvious thing).

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:345
>> +        # Always add Skia test_expectations overrides, if they exist.
> 
> Patch #2 adds the Skia test_expectations file on *all* builders, while maintaining the current behavior of only adding the Chromium test_expectations file on *certain* builders.
> 
> The difference in behavior is because expectations are only supposed to live in the Chromium test_expectations file "on their way" to living in the main webkit test_expectations file, while the Skia test_expectations should NOT be migrated over by gardeners.
> 
> Ready for review.

This behavior seems fine to me, this comment doesn't add much though == another what comment.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:350
> +        # Only add Chromium test_expectations on certain builders.

Ditto: re: what comments. The code pretty clearly shows that we only do this on certain builders, so this comment doesn't add value.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:359
> +        # Add base-platform overrides last.

Ditto.

[Elliot Poger]

Created attachment 142550 [details]
Patch

[Elliot Poger]

Comment on attachment 142538 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:323
>> +    def get_expectations_file_contents(self, filetype, filepath):
> 
> Nit: webkit generally avoids putting "get" in method names. expectations_file_contents would be fine. A lot of the layout_tests code doesn't adhere to this since it came from the Chromium tree initially, but we're slowly moving it over to a more webkitty style.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:330
>> +        """
> 
> Ditto: webkit is allergic to "what" comments. This comment states what the code does pretty clearly if you take a quick look at it. Comments in webkit should only be to explain why (e.g. why are we doing some crazy, non-obvious thing).

Removed.

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:345
>>> +        # Always add Skia test_expectations overrides, if they exist.
>> 
>> Patch #2 adds the Skia test_expectations file on *all* builders, while maintaining the current behavior of only adding the Chromium test_expectations file on *certain* builders.
>> 
>> The difference in behavior is because expectations are only supposed to live in the Chromium test_expectations file "on their way" to living in the main webkit test_expectations file, while the Skia test_expectations should NOT be migrated over by gardeners.
>> 
>> Ready for review.
> 
> This behavior seems fine to me, this comment doesn't add much though == another what comment.

Removed the comment.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:350
>> +        # Only add Chromium test_expectations on certain builders.
> 
> Ditto: re: what comments. The code pretty clearly shows that we only do this on certain builders, so this comment doesn't add value.

Removed.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:359
>> +        # Add base-platform overrides last.
> 
> Ditto.

OK, even I think this comment was overkill. Removed.

[Elliot Poger]

Created attachment 142718 [details]
Patch

[Elliot Poger]

Comment on attachment 142718 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:339
> +                'skia', 'skia_test_expectations.txt'))

In this latest patch, I made just one change... I changed the path to Skia's test_expectations file so that it lives within the Chrome repository instead of the Skia repository.  I realized this morning that doing so would fit better with our process for rolling Skia changes into Chrome and WebKit.

See https://chromiumcodereview.appspot.com/10387188 for my addition of this file to the Chrome repo.

[Dirk Pranke]

Comment on attachment 142718 [details]
Patch

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

I'm a little concerned about having the skia overrides live in the chromium repo and still apply to all builds, rather than just the deps bots; this means that we can't be sure that all the suppressions necessary for a green roll are present in just the upstream files; they're no way to ignore the skia suppressions. The chromium overrides initially applied everywhere, but we ended up changing that for just this reason.

That said, it's not clear to me that there's a better solution, and in theory most of the time the skia overrides will be empty, so this won't be an issue. I'm willing to give this a shot.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:323
> +    def expectations_file_contents(self, filetype, filepath):

Nit: I think this is a protected method, right? It should be _expectations_file_contents (leading underscore).

[Ojan Vafai]

(In reply to comment #17)
> (From update of attachment 142718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142718&action=review
> 
> That said, it's not clear to me that there's a better solution, and in theory most of the time the skia overrides will be empty, so this won't be an issue. I'm willing to give this a shot.

I realized I was making the same assumption. Elliot, you do intend that most of the time the file will be empty, right? As in, it will only have entries around the time of a skia roll. Any longer-lived entries would move the to regular test_expectations.txt file, right? I'd like the file to have a big comment at the top emphasizing this.

[Elliot Poger]

Created attachment 142752 [details]
Patch

[Elliot Poger]

Comment on attachment 142718 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:323
>> +    def expectations_file_contents(self, filetype, filepath):
> 
> Nit: I think this is a protected method, right? It should be _expectations_file_contents (leading underscore).

Fixed.

[Elliot Poger]

(In reply to comment #18)
> > That said, it's not clear to me that there's a better solution, and in theory most of the time the skia overrides will be empty, so this won't be an issue. I'm willing to give this a shot.
> 
> I realized I was making the same assumption. Elliot, you do intend that most of the time the file will be empty, right? As in, it will only have entries around the time of a skia roll. Any longer-lived entries would move the to regular test_expectations.txt file, right? I'd like the file to have a big comment at the top emphasizing this.

Yes, most of the time the skia_test_expectations.txt file will be empty.

I have added you (Ojan) as a reviewer on https://chromiumcodereview.appspot.com/10387188/ ('Add skia_test_expectations file').  I will make a couple of adjustments to that in a few minutes; once I have done so, please suggest any further adjustments you would like to see.

[Ojan Vafai]

Hmmm...thinking about this more, there's a downside that hadn't occurred to me. The chromium bots at build.webkit.org will fall all the tests listed in this file if the skia version on those bots is updated. Any ideas how we can avoid that?

[Dirk Pranke]

(In reply to comment #22)
> Hmmm...thinking about this more, there's a downside that hadn't occurred to me. The chromium bots at build.webkit.org will fall all the tests listed in this file if the skia version on those bots is updated. Any ideas how we can avoid that?

You're worried if we roll skia on those bots but not roll chromium? We pull the skia rev from the chromium deps file, so it seems like that can't happen?

[Ojan Vafai]

(In reply to comment #23)
> (In reply to comment #22)
> > Hmmm...thinking about this more, there's a downside that hadn't occurred to me. The chromium bots at build.webkit.org will fall all the tests listed in this file if the skia version on those bots is updated. Any ideas how we can avoid that?
> 
> You're worried if we roll skia on those bots but not roll chromium? We pull the skia rev from the chromium deps file, so it seems like that can't happen?

No. Those bots don't have a chromium checkout, so they don't have the new skia test_expectations.txt file. I suppose we could make it part of the DEPS for those bots?

[Dirk Pranke]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > Hmmm...thinking about this more, there's a downside that hadn't occurred to me. The chromium bots at build.webkit.org will fall all the tests listed in this file if the skia version on those bots is updated. Any ideas how we can avoid that?
> > 
> > You're worried if we roll skia on those bots but not roll chromium? We pull the skia rev from the chromium deps file, so it seems like that can't happen?
> 
> No. Those bots don't have a chromium checkout, so they don't have the new skia test_expectations.txt file. I suppose we could make it part of the DEPS for those bots?

They have all the parts of chrome needed to actually build DRT, and that includes skia (and hence would include the new skia test_expectations file).

[Elliot Poger]

(In reply to comment #24)
> No. Those bots don't have a chromium checkout, so they don't have the new skia test_expectations.txt file. I suppose we could make it part of the DEPS for those bots?

Oh, lordy.  The chromium bots at build.webkit.org don't have a chromium checkout?  Seems very strange.

Can you please point me at the DEPS file used by those bots?  I'm curious as to how they are selecting their Skia revision.

[Dirk Pranke]

(In reply to comment #26)
> (In reply to comment #24)
> > No. Those bots don't have a chromium checkout, so they don't have the new skia test_expectations.txt file. I suppose we could make it part of the DEPS for those bots?
> 
> Oh, lordy.  The chromium bots at build.webkit.org don't have a chromium checkout?  Seems very strange.
> 
> Can you please point me at the DEPS file used by those bots?  I'm curious as to how they are selecting their Skia revision.

http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS#L56

They pick up the version of skia specified in chromium's deps file. Ignore Ojan :).

[Ojan Vafai]

(In reply to comment #25)
> They have all the parts of chrome needed to actually build DRT, and that includes skia (and hence would include the new skia test_expectations file).

Eliot's recent change move it to the chromium repo. Eliot, maybe this is a good reason to keep it in the skia repo?

(In reply to comment #26)
> Oh, lordy.  The chromium bots at build.webkit.org don't have a chromium checkout?  Seems very strange.

It makes sense for the goal of being able to build and run the chromium ports from a pure webkit checkout, which is important for having non-chromium webkit contributors be able to checkout and build the chromium port.

[Ojan Vafai]

Oh, we pull in the chromium skia directory from DEPS. Dirk is right, ignore me.

[Elliot Poger]

(In reply to comment #29)
> Oh, we pull in the chromium skia directory from DEPS. Dirk is right, ignore me.

Ignoring you.

For the record, there is a strong reason to keep that file in the Chromium repo... it will work much better with the process of rolling Skia changes into Chromium (and, by extension, WebKit).  Basically: it's when we test the Skia DEPS roll into Chromium that we will typically discover the broken layout tests, so it's best if the expectations change is in the same CL as that DEPS roll.

[Elliot Poger]

Comment on attachment 142752 [details]
Patch

Removing from the slooow commitqueue for now, since it's 5pm on a Friday.  I'll put it back on the commitqueue Monday morning.

[Elliot Poger]

Comment on attachment 142752 [details]
Patch

back into the commit-queue...

[WebKit Review Bot]

Comment on attachment 142752 [details]
Patch

Rejecting attachment 142752 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12736553

[Elliot Poger]

Created attachment 143042 [details]
Patch

[Elliot Poger]

Comment on attachment 143042 [details]
Patch

manually added "Reviewed by Dirk Pranke" to ChangeLog, trying the commit-queue again...

[WebKit Review Bot]

Comment on attachment 143042 [details]
Patch

Clearing flags on attachment: 143042

Committed r117789: <http://trac.webkit.org/changeset/117789>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.