Bug 88456 – get rebaselining tools to kinda work with the skia overrides file

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

[Dirk Pranke]

We recently added support for a "skia" overrides file for the chromium port, so that the skia devs can more easily manage skia changes that require a lot of rebaselining (bug 86749).

Unfortunately, that broke how rebaselining worked for the main files (see bug 87406).

The root of the problem is that the test expectations code does not maintain a mapping of actual expectation entries to the text files they are found in. So, when we need to write out an updated file after removing a REBASELINE entry, we serialize all of the entries (across all of the files) into a single text file. This was bad because it would take the contents of the skia file and insert them into the main expectations file :(.

So, in bug 87406 we disabled looking at the overrides files altogether during rebaselining, which fixes the normal/gardener use case for rebaselining, but breaks the skia dev use case.

One correct long term fix would be to maintain the entry<->file mapping for all of the entries; we would need this in order to support cascaded expectations files. Of course, another approach would be to stop using the REBASELINE keyword in the file, but we'll probably need the mapping to make cascading maintainable/debuggable regardless.

Cascading expectations are being tracked in bug 65834.

In the meantime, in order to unbreak the skia devs, I'm going to add a switch that will optionally restore the "broken" behavior we had in 86749; this seems to be an awkward but acceptable workflow for the skia devs and is a minimal amount of work, allowing me to focus on fixing 64834.

[Dirk Pranke]

er, that's 65834, not 64834.

[Ryosuke Niwa]

(In reply to comment #0)
> In the meantime, in order to unbreak the skia devs, I'm going to add a switch that will optionally restore the "broken" behavior we had in 86749; this seems to be an awkward but acceptable workflow for the skia devs and is a minimal amount of work, allowing me to focus on fixing 64834.

I don't see how adding the broken behavior back using a command line switch will help. How does copying skia_test_expectations.txt entries into TestExpectations for every rebaseline we do ever help? Skia folks can already rebaseline tests as expected because tests marked failing in skia_test_expectations.txt still show up on garden-o-matic's expected failures' tab, and you can still rebaseline those tests successfully only updating entries in test_expectations.txt.

We had never supported skia_test_expectations.txt so introducing the old behavior isn't going to help skia folks at all.

[Dirk Pranke]

(In reply to comment #2)
> (In reply to comment #0)
> > In the meantime, in order to unbreak the skia devs, I'm going to add a switch that will optionally restore the "broken" behavior we had in 86749; this seems to be an awkward but acceptable workflow for the skia devs and is a minimal amount of work, allowing me to focus on fixing 64834.
> 
> I don't see how adding the broken behavior back using a command line switch will help. How does copying skia_test_expectations.txt entries into TestExpectations for every rebaseline we do ever help? Skia folks can already rebaseline tests as expected because tests marked failing in skia_test_expectations.txt still show up on garden-o-matic's expected failures' tab, and you can still rebaseline those tests successfully only updating entries in test_expectations.txt.
> 
> We had never supported skia_test_expectations.txt so introducing the old behavior isn't going to help skia folks at all.

As discussed over IRC, the point is to ensure that if the skia devs put "REBASELINE" into skia_test_expectations.txt, we'll actually rebaseline the test. They don't care about garbage getting written into TestExpectations, because they won't commit that change, and they're okay with skia_test_expectations.txt not getting updated properly for now.

Also, you correctly noted that we don't actually need a flag to get the right behavior. in the rebaseline-expectations command, as long as we include the overrides when getting the list of files to rebaseline, but don't include them when writing the expectations back out, we should be okay.

[Dirk Pranke]

Created attachment 146130 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 146130 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:264
> +    def _expectations(self, port, include_overrides):
> +        return TestExpectations(port, include_overrides=include_overrides)

Do we really need this method still?

[Dirk Pranke]

Committed r119628: <http://trac.webkit.org/changeset/119628>

[Dirk Pranke]

(In reply to comment #5)
 
> Do we really need this method still?

Probably not :).

[Ojan Vafai]

(In reply to comment #0)
> The root of the problem is that the test expectations code does not maintain a mapping of actual expectation entries to the text files they are found in. So, when we need to write out an updated file after removing a REBASELINE entry, we serialize all of the entries (across all of the files) into a single text file. This was bad because it would take the contents of the skia file and insert them into the main expectations file :(.

Rebaseline entry aside, we want to have automated tooling like garden-o-matic be able to remove entries from the right file when we have multiple cascading files in the tree.