Bug 96718 – mac-future ignores fallback test expectations

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

[Stephanie Lewis]

When running tests on mac-future TestExpectation files from mac are ignored.

The configuration matching is using a fallback array that doesn't contain mac-future so no expectation configurations match

patch coming.

[Stephanie Lewis]

Created attachment 164028 [details]
patch

[Dirk Pranke]

1) I think there's something wrong with your patch (wrong mime type?)

2) mac-future isn't supposed to be a real test configuration, so I'm a bit nervous with this change. What are you running that's giving you the error?

[Stephanie Lewis]

Created attachment 164034 [details]
patch.txt?

weird, it was just the output from git format-patch

[Stephanie Lewis]

There were comments in the code that suggested we need to refactor the approach as well.  I'm open to that, but if this is going to be a large refactoring I'd like to get my tests running first.

My understanding was that mac-future was added as a placeholder for platforms still in development.  Running the layout tests against changes to mac os early on helps us find regressions in underlying code.

We use 

--additional-platform-directory=ADDITIONAL_PLATFORM_DIRECTORY
                        Additional directory where to look for test baselines
                        (will take precendence over platform baselines).
                        Specify multiple times to add multiple search path
                        entries.
--additional-expectations=ADDITIONAL_EXPECTATIONS
                        Path to a test_expectations file that will override
                        previous expectations. Specify multiple times for
                        multiple sets of overrides.

to specify the location of those results and the TestExpectations file.

The bug is that since there is no match for the platform in VERSION_FALLBACK_ORDER the expectations for older platforms don't match configurations.

[Dirk Pranke]

(In reply to comment #4)
> There were comments in the code that suggested we need to refactor the approach as well.  I'm open to that, but if this is going to be a large refactoring I'd like to get my tests running first.
> 
> My understanding was that mac-future was added as a placeholder for platforms still in development.  Running the layout tests against changes to mac os early on helps us find regressions in underlying code.
> 

Not exactly. "mac-future" is a placeholder for platforms still in development, but really it's meant to always resolve to platform/mac (as opposed to something like "mac-lion"  which may point to platform/mac one day and platform/mac-lion the next. So, the theory was that platform/mac would always contain the results of whatever was under development and, if the currently shipping version produces different results, it would go in mac-lion or whatever (mac-mountainlion now).

I believe I worked this all out w/ Mark Rowe a while ago. That said, I'm not particularly wedded to this and we can certainly change it to whatever works for you.

> We use 
> 
> --additional-platform-directory=ADDITIONAL_PLATFORM_DIRECTORY
>                         Additional directory where to look for test baselines
>                         (will take precendence over platform baselines).
>                         Specify multiple times to add multiple search path
>                         entries.
> --additional-expectations=ADDITIONAL_EXPECTATIONS
>                         Path to a test_expectations file that will override
>                         previous expectations. Specify multiple times for
>                         multiple sets of overrides.
> 
> to specify the location of those results and the TestExpectations file.
> 
> The bug is that since there is no match for the platform in VERSION_FALLBACK_ORDER the expectations for older platforms don't match configurations.

I'm having trouble parsing this sentence. Why do you need mac-future to be in VERSION_FALLBACK_ORDER if you're also using --additional-expectations and --additional-platform-directory? Does run-webkit-tests --platform mac-future --additional-platform-directory=X --additional-expectations=Y not work? I tried this quickly before I left the office tonight and --platform mac-future seemed to work ok for me, but I might've missed something.

[Stephanie Lewis]

(In reply to comment #5)
> (In reply to comment #4)
> > There were comments in the code that suggested we need to refactor the approach as well.  I'm open to that, but if this is going to be a large refactoring I'd like to get my tests running first.
> > 
> > My understanding was that mac-future was added as a placeholder for platforms still in development.  Running the layout tests against changes to mac os early on helps us find regressions in underlying code.
> > 
> 
> Not exactly. "mac-future" is a placeholder for platforms still in development, but really it's meant to always resolve to platform/mac (as opposed to something like "mac-lion"  which may point to platform/mac one day and platform/mac-lion the next. So, the theory was that platform/mac would always contain the results of whatever was under development and, if the currently shipping version produces different results, it would go in mac-lion or whatever (mac-mountainlion now).

I think our issue is that we now have two "macs".  The one that webkit considers the most recent and the one that we consider the most recent.  I'm confused as to how you think we should differentiate between the two.

> 
> I believe I worked this all out w/ Mark Rowe a while ago. That said, I'm not particularly wedded to this and we can certainly change it to whatever works for you.
> 
> > We use 
> > 
> > --additional-platform-directory=ADDITIONAL_PLATFORM_DIRECTORY
> >                         Additional directory where to look for test baselines
> >                         (will take precendence over platform baselines).
> >                         Specify multiple times to add multiple search path
> >                         entries.
> > --additional-expectations=ADDITIONAL_EXPECTATIONS
> >                         Path to a test_expectations file that will override
> >                         previous expectations. Specify multiple times for
> >                         multiple sets of overrides.
> > 
> > to specify the location of those results and the TestExpectations file.
> > 
> > The bug is that since there is no match for the platform in VERSION_FALLBACK_ORDER the expectations for older platforms don't match configurations.
> 
> I'm having trouble parsing this sentence. Why do you need mac-future to be in VERSION_FALLBACK_ORDER if you're also using --additional-expectations and --additional-platform-directory? Does run-webkit-tests --platform mac-future --additional-platform-directory=X --additional-expectations=Y not work? I tried this quickly before I left the office tonight and --platform mac-future seemed to work ok for me, but I might've missed something.

passing --platform doesn't resolve the TestExpectation issue.

So what I tracked down the problem to is that we create a TestExpectation for each port that we want to use the testexpectations files for.  Those are mac-future and  mac.  mac is what we are interested in here.  TestExpectation has a list of configurations that the mac testexpectation would apply to.  The function to create that list is port._generate_all_test_configurations.  The configurations end up being VERSION_FALLBACK_ORDER x BUILD_TYPES x ARCHITECTURES.  Since the platform we're running is mac-future and the VERSION_FALLBACK_ORDER doesn't contain mac-future TestExpectation decides that the mac testexpectation doesn't apply to our current platform.

[Dirk Pranke]

I'm sorry, I'm surely missing something here. 

First, if this discussion is blocking anything important, feel free to have someone r+ this and land it while we try to resolve my confusion; the patch looks fine but I don't want to r+ something I don't understand the need for.

I understand the desire to distinguish between the version that is currently shipping and some internal unreleased version. I believe that in order to run run-webkit-tests with some new set of tests, baselines, and expectations, running with --platform mac-future --additional-platform-dir=X --additional-expectations=Y is all you should need. That will pick up the TestExpectations in platform/mac in addition to the file you've specified on the command line. You can verify this by, e.g., running with --details and checking the expectation for some test listed in mac/TestExpectations, like fast/dom/shadow/content-element-api.html.

The point I'm confused about is your need to modify _generate_all_test_configurations(); while that function is called during run-webkit-tests, it isn't (I don't think) really used for anything, and I don't think adding mac-future to that will have any effect on how run-webkit-tests runs. 

The list of configurations is used mostly (solely?) when using the 'REBASELINE' modifier and rewriting the file when tests no longer fail. So, I could believe that mac-future needs to be in that list if you're trying to make it work with garden-o-matic or webkit-patch rebaseline-expectations. Are you trying to use one of those commands, or are you having some problem with run-webkit-tests that I'm not seeing?

[Stephanie Lewis]

_generate_all_test_configurations is called during the test to when parsing test_expectations.  Starting from the failing line I outlined how this is failing for us.


The problem line is in test_expectations.py in def _add_expectations(self, expectation_list):

if self._is_lint_mode or self._test_config in expectation_line.matching_configurations:
    self._model.add_expectation_line(expectation_line)

_log.debug('is_lint_mode=%s _test_config=%s matching_config=%s' % (self._is_lint_mode, self._test_config, expectation_line.matching_configurations))

The log statement outputs this:

14:08:15.730 12586 is_lint_mode=False _test_config=<future, x86_64, release> matching_config=set([TestConfig(version='snowleopard', architecture='x86', build_type='debug'), TestConfig(version='mountainlion', architecture='x86_64', build_type='debug'), TestConfig(version='lion', architecture='x86', build_type='debug'), TestConfig(version='mountainlion', architecture='x86', build_type='release'), TestConfig(version='lion', architecture='x86_64', build_type='debug'), TestConfig(version='mountainlion', architecture='x86', build_type='debug'), TestConfig(version='snowleopard', architecture='x86_64', build_type='debug'), TestConfig(version='lion', architecture='x86', build_type='release'), TestConfig(version='snowleopard', architecture='x86', build_type='release'), TestConfig(version='lion', architecture='x86_64', build_type='release'), TestConfig(version='mountainlion', architecture='x86_64', build_type='release'), TestConfig(version='snowleopard', architecture='x86_64', build_type='release')])

you'll notice that the _test_config is future and the matching_configurations don't have future as an option.  So the expectation for this test is never added.

expectation_line.matching_configurations is set in test_expectations.py in_parse_modifiers(self, expectation_line):

expectation_line.matching_configurations = self._test_configuration_converter.to_config_set(parsed_specifiers, expectation_line.warnings)

the test_configurarion_converter has the list of configurations and it is created in test_expectations.py in TestExpectationParser.__init__(self, port, full_test_list, allow_rebaseline_modifier)

self._test_configuration_converter = TestConfigurationConverter(set(port.all_test_configurations()), port.configuration_specifier_macros())

port.all_test_configurations() calls _generate_all_test_configurations which uses the VERSION_FALLBACK_ORDER to come up with a list of platforms.

[Dirk Pranke]

Okay, apparently I've been misreading the logs this whole time ... sigh.

I've reproduced your problem. Patch is fine.

[Stephanie Lewis]

Committed http://trac.webkit.org/projects/webkit/changeset/128662