Bug 37565 – new-run-webkit-tests needs a shared TestExpectations between all WebKit ports

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

[Eric Seidel (no email)]

new-run-webkit-tests needs a shared test_expectations between all ports

Some tests in WebKit's repository need to be marked SLOW.  They need to be marked slow for all ports.  Seems the best way to do that is with a common text_expectations file.

I'm not sure the current architecture is designed to support such.  This might require a bit of work.

[Dirk Pranke]

The test_expectations and port/ structure was designed to share test_expectations.txt files where possible, but there is a tradeoff between sharing results across all ports and sharing results across a few ports (e.g., if we were to share text_expectations.txt between WebKit Mac and Chromium Mac, then we would have to refer to port-specific diffs as "MAC" and "CHROMIUM-MAC". This may make people unhappy).

I think it's more of a social question than a technical question. Technically, sharing a single file would require us to have a common set of test_platform_names() used by all the ports, and changing the path to the test_expectations file to use it.

One other alternative we could do would be to support multiple test_expectations files, and perhaps have a "generic" file that didn't have port-specific or platform-specific expectations. This would perhaps only apply to SLOW tests and CRASHes (and could maybe be used for REBASELINE).

[Eric Seidel (no email)]

Sorry, I wasn't clear.  I meant supporting multiple test_expectations.txt files, with a hierarchy.

Both:
LayoutTests/platform/test_expectations.txt
and
LayoutTests/platform/mac/test_expectations.txt

I'm just not sure how the layering would work.  If a more specific expectation is given in a more specific file do we override the old expectation?  Add to it?  When do we warn about duplicate expectations?

[Evan Martin]

The danger of a hierarchy is that you have non-local effects with your edits.  Say something starts failing on a "core" port, so it's marked fail there; now all pieces that derive from it are suddenly expected-fail.

[Eric Seidel (no email)]

The current solution is OK, we just end up with a lot of boilerplate at the top of every port file for marking tests as slow or flaky (since a bunch of them are on all ports).  It makes bringing up additional ports on new-run-webkit-tests challenging.

[Dirk Pranke]

Note that we do effectively already have a multi-level set of expectations on the Chromium ports, since we support having both upstream and downstream files which are effectively just concatenated. We could do this for WebKit as well - have a generic and a platform file and cat them together, and bail out on conflicts. 

This would require us to never add an entry to the generic file if a platform might override it, but maybe that's okay?

Implementing something where the platform can override a generic result would be more complicated.

[Eric Seidel (no email)]

I think what we have now (duplicating SLOW tests through all ports is OK.

I think that having a model where we couldn't ever "override" in ports, but rather had to copy any values we wanted to override to all ports would be strictly better than what we have today.

Eventually maybe we'd make something more sophisticated, not sure.

[Dirk Pranke]

We now support cascading expectations, so we should be able to implement this.

Note that we don't support more-specific expectations overriding less-specific expectations, and we don't necessarily support the same list of keywords across ports, so the only way this would work at the moment is if the top-level file only contained tests that had the same expectations across all ports regardless of configuration (or if they were all in the same configuration).

Possible examples:

1) tests that are slow but pass everywhere
2) tests that crash, timeout or should be skipped everywhere
3) tests that crash, timout or should be skipped only in debug mode (e.g., a check that fires everywhere)
4) tests that fail with the same TEXT failure everywhere (?)

[Glenn Adams]

Created attachment 187924 [details]
Patch

[Dirk Pranke]

Comment on attachment 187924 [details]
Patch

I would expect this file to live in LayoutTests/TestExpectation, not LayoutTests/platform/TestExpectations, but this is a minor thing.

[Glenn Adams]

(In reply to comment #9)
> (From update of attachment 187924 [details])
> I would expect this file to live in LayoutTests/TestExpectation, not LayoutTests/platform/TestExpectations, but this is a minor thing.

I started to implement it that way per your prior suggestion on webkit-dev on this thread; however, I found it required many more changes due to assumptions by various code that all expectation files live under the platform directory. It would have made the patch rather more complicated to put there.

I would suggest we start with this simpler approach (under platform), start using it a bit, and the re-visit the question of whether it would work better at one level up, in which case we could move in a follow-on patch.

WDYT?

[Dirk Pranke]

It's hard for me to say without having really knowing what the code changes  necessary to move it out of platform/ would be. I'm kinda surprised that there'd be much difference.

I don't think we should be trying to minimize the changes to the code and sacrifice user convenience for this, and I do think creating the file and then moving it would be kind of inconvenient, so therefore I'm not a fan of landing this and planning to move it later. 

This means that we should probably either take the bigger hit now to the code, or just land this as-is. Do you have any of your earlier work still around so I can get a sense of more of the details? If not, maybe I should spend at least a little time trying to implement it myself so I can get a better sense.

[Glenn Adams]

(In reply to comment #11)
> It's hard for me to say without having really knowing what the code changes  necessary to move it out of platform/ would be. I'm kinda surprised that there'd be much difference.
> 
> I don't think we should be trying to minimize the changes to the code and sacrifice user convenience for this, and I do think creating the file and then moving it would be kind of inconvenient, so therefore I'm not a fan of landing this and planning to move it later. 
> 
> This means that we should probably either take the bigger hit now to the code, or just land this as-is. Do you have any of your earlier work still around so I can get a sense of more of the details? If not, maybe I should spend at least a little time trying to implement it myself so I can get a better sense.

Nope, I nuked it. In the current patch, you will notice that by using an empty string as the search path entry for constructing the result of expectation_files() that it makes it very straightforward to introduce the generic expectations file. One way to address that would be to add a post-processing phase for every implementation of expectations_files() that rewrites "/platform/TestExpectations" as "/TestExpectations".

I could give that a shot if you like.

[Dirk Pranke]

(In reply to comment #12)
> (In reply to comment #11)
> > It's hard for me to say without having really knowing what the code changes  necessary to move it out of platform/ would be. I'm kinda surprised that there'd be much difference.
> > 
> > I don't think we should be trying to minimize the changes to the code and sacrifice user convenience for this, and I do think creating the file and then moving it would be kind of inconvenient, so therefore I'm not a fan of landing this and planning to move it later. 
> > 
> > This means that we should probably either take the bigger hit now to the code, or just land this as-is. Do you have any of your earlier work still around so I can get a sense of more of the details? If not, maybe I should spend at least a little time trying to implement it myself so I can get a better sense.
> 
> Nope, I nuked it. In the current patch, you will notice that by using an empty string as the search path entry for constructing the result of expectation_files() that it makes it very straightforward to introduce the generic expectations file. One way to address that would be to add a post-processing phase for every implementation of expectations_files() that rewrites "/platform/TestExpectations" as "/TestExpectations".
> 
> I could give that a shot if you like.

Yeah, I saw that and wasn't a big fan of that approach either, it's kinda mysterious.

I wonder if this is another good motivator for me to attempt to refactor that code again and get everybody using more common logic for this.

[Glenn Adams]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > It's hard for me to say without having really knowing what the code changes  necessary to move it out of platform/ would be. I'm kinda surprised that there'd be much difference.
> > > 
> > > I don't think we should be trying to minimize the changes to the code and sacrifice user convenience for this, and I do think creating the file and then moving it would be kind of inconvenient, so therefore I'm not a fan of landing this and planning to move it later. 
> > > 
> > > This means that we should probably either take the bigger hit now to the code, or just land this as-is. Do you have any of your earlier work still around so I can get a sense of more of the details? If not, maybe I should spend at least a little time trying to implement it myself so I can get a better sense.
> > 
> > Nope, I nuked it. In the current patch, you will notice that by using an empty string as the search path entry for constructing the result of expectation_files() that it makes it very straightforward to introduce the generic expectations file. One way to address that would be to add a post-processing phase for every implementation of expectations_files() that rewrites "/platform/TestExpectations" as "/TestExpectations".
> > 
> > I could give that a shot if you like.
> 
> Yeah, I saw that and wasn't a big fan of that approach either, it's kinda mysterious.
> 
> I wonder if this is another good motivator for me to attempt to refactor that code again and get everybody using more common logic for this.

Well, I have to admit that I found the current code rather in need of refactoring. For example, every port (except WinPort) overrides expectations_files(), while WinPort relies on the base class (Port) implementation. Further, pretty much all of these implementations appear to be doing the same thing but in slightly different ways.

My preference would be to land this patch (after I add the post-processing step as suggested above), and then you (or someone else with enough interest) could do the refactoring per time available.

Right now, I actually have a need for the generic expectations file, so I'd like to see it landed sooner than perhaps may be required if a rewrite is performed.

[Dirk Pranke]

let me take a more detailed look this afternoon and get back to you. I'd prefer not to land this patch until then.

[Glenn Adams]

(In reply to comment #15)
> let me take a more detailed look this afternoon and get back to you. I'd prefer not to land this patch until then.

ok, thanks!

[Dirk Pranke]

Okay, I took a quick-ish look. I think the thing to do is to rename the existing implementations of expectation_files() to _port_specific_expectation_files(), and provide a generic implementation in base.py that returns [self.relative_test_filename('TestExpectations')] + self._port_specific_expectation_files(), and then update the tests.

The path_to_test_expectations_file() will still return the "default" port-specific file, and the rebaselining commands in webkit-patch will still be broken w/ multiple files, but I'm not sure that the changes I'm describing here will really make these things worse - we really just need to fix bug 95390.

At any rate, I think the changes described above should work, should be no bigger than your current patch, and should be fairly forward-going. Does this make sense, and do you want to take a stab at it, or would you prefer me to do it (I probably won't be able to get to it until later this evening)?

[Glenn Adams]

(In reply to comment #17)
> Okay, I took a quick-ish look. I think the thing to do is to rename the existing implementations of expectation_files() to _port_specific_expectation_files(), and provide a generic implementation in base.py that returns [self.relative_test_filename('TestExpectations')] + self._port_specific_expectation_files(), and then update the tests.
> 
> The path_to_test_expectations_file() will still return the "default" port-specific file, and the rebaselining commands in webkit-patch will still be broken w/ multiple files, but I'm not sure that the changes I'm describing here will really make these things worse - we really just need to fix bug 95390.
> 
> At any rate, I think the changes described above should work, should be no bigger than your current patch, and should be fairly forward-going. Does this make sense, and do you want to take a stab at it, or would you prefer me to do it (I probably won't be able to get to it until later this evening)?

sounds good; i'll handle it then update patch later today

[Glenn Adams]

Created attachment 188151 [details]
Patch

[Dirk Pranke]

Comment on attachment 188151 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:155
> +            expectations = TestExpectations(port, include_generic=False, include_overrides=False)

interesting. I missed that this was doing include_overrides=False before. I don't like having to add include_generic, but it is consistent and does make some things easier. I think this is fine for now until I can clean up how rebaselining handles the cascade as a whole.

[Glenn Adams]

Comment on attachment 188151 [details]
Patch

i believe this is ready for CQ; the mac-ews fail appears to be unrelated

[WebKit Review Bot]

Comment on attachment 188151 [details]
Patch

Clearing flags on attachment: 188151

Committed r142941: <http://trac.webkit.org/changeset/142941>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.