Bug 36249 – new-run-webkit-tests should support upstream and downstream expectations

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

If you have a port that is built and running out of two repositories like Chromium is, then it can be useful to have both "upstream" and "downstream" expectations in case you make a change downstream (in the Chromium repository) that changes the test results in such a way as to conflict with the upstream baselines and expectations.

There are two ways in which this might happen:

1) If I fix a bug downstream that causes a test expected to fail upstream to no longer fail. In the best case, the test now passes, and we get an "unexpected pass" which is a little annoying but can be ignored. In the worse case, a test goes from a more severe failure to a better failure (crash -> text diff). To address this, either you could check in a downstream baseline, or add a downstream expectation that overrides an upstream expectation.

2) If I make a change downstream that causes a test expected to pass upstream to start failing (or failing worse). Usually, we would revert the downstream change, but sometimes this may not be possible. In this case you can either add a new downstream baseline (which wouldn't work for a crash or a timeout), or add an overriding expectation.

In both of these cases, adding a new baseline is probably the wrong thing to do. There is a variant on this, where the upstream baseline is actually no longer correct and needs to change.

Ideally all the baselines live in one place, so we'd like to avoid having to create downstream repos if possible. Also, in some cases, working around the problem by changing the baseline either won't work or is just a subpar thing to do.

So, we should add the ability to override expectations.

[Dirk Pranke]

Created attachment 51007 [details]
Patch

[Ojan Vafai]

Comment on attachment 51007 [details]
Patch

> +
> +        if not self._suppress_errors and (
> +            len(self._errors) or len(self._non_fatal_errors)):
> +            if self._is_debug_mode:
> +                build_type = 'DEBUG'
> +            else:
> +                build_type = 'RELEASE'
> +            _log.error('')
> +            _log.error("FAILURES FOR PLATFORM: %s, BUILD_TYPE: %s" %
> +                       (self._test_platform_name.upper(), build_type))
> +
> +            for error in self._non_fatal_errors:
> +                _log.error(error)
> +            _log.error('')
> +
> +            if len(self._errors):
> +                raise SyntaxError('\n'.join(map(str, self._errors)))
> +
> +
> +        # Now add in the tests that weren't present in the expectations file.
> +        expectations = set([PASS])
> +        options = []
> +        modifiers = []
> +        if self._full_test_list:
> +            for test in self._full_test_list:
> +                if not test in self._test_list_paths:
> +                    self._add_test(test, modifiers, expectations, options,
> +                        overrides_allowed=False)

I know you're just moving this code, but maybe move these into helper functions (e.g. logErrors() and addTestsExpectedToPass())? I'm more allergic to large blocks of code in constructors. 

> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations_test.py

unittests!!! awesome.

> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
> @@ -437,6 +437,15 @@ class Port(object):
>          test_expectations file. See test_expectations.py for more details."""
>          raise NotImplementedError('Port.test_expectations')
>  
> +    def test_expectations_overrides(self):
> +        """Returns an optional set of overrides for the test_expectations.
> +
> +        This is used by ports that have code in two repositories, and where
> +        is is possible that you might need "downstream" expectations that

s/is is/it is

> +        temporarily override the "upstream" expectations until the port can
> +        sync up the two repos."""
> +        return None
> +
>      def test_base_platform_names(self):
>          """Return a list of the 'base' platforms on your port. The base
>          platforms represent different architectures, operating systems,
> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py
> index 13caf4a..3ada317 100644
> --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py
> @@ -114,8 +114,8 @@ class ChromiumPort(base.Port):
>          return os.path.join(self._chromium_base_dir, *comps)
>  
>      def path_to_test_expectations_file(self):
> -        return self.path_from_chromium_base('webkit', 'tools', 'layout_tests',
> -                                            'test_expectations.txt')
> +        return self.path_from_webkit_base('LayoutTests', 'platform',
> +            'chromium', 'test_expectations.txt')

Did you mean to include this in this patch? Seems like this should go along with actually upstreaming the file.

>      def results_directory(self):
>          return self.path_from_chromium_base('webkit', self._options.target,
> @@ -163,7 +163,20 @@ class ChromiumPort(base.Port):
>          Basically this string should contain the equivalent of a
>          test_expectations file. See test_expectations.py for more details."""
>          expectations_file = self.path_to_test_expectations_file()
> -        return file(expectations_file, "r").read()
> +        if os.path.exists(expectations_file):
> +            return file(expectations_file, "r").read()
> +        else:
> +            return ''

We should fail hard if the expectations file doesn't exist. Until we upstream the expectations file itself, lets keep pointing at the downstream one so we don't have this problem. Or, you could create an empty expectations file upstream, but it seems weird to me that in the limbo period, the overrides file will be the primary expectations file.

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_linux.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_linux.py
> index e147bfd..a3ce15f 100644
> --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_linux.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_linux.py
> @@ -51,8 +51,11 @@ class ChromiumLinuxPort(chromium.ChromiumPort):
>          chromium.ChromiumPort.__init__(self, port_name, options)
>  
>      def baseline_search_path(self):
> -        return [self.baseline_path(),
> +        return [self._chromium_baseline_path('chromium-linux'),
>                  self._chromium_baseline_path('chromium-win'),
> +                self._webkit_baseline_path('chromium-linux'),
> +                self._webkit_baseline_path('chromium-win'),
> +                self._webkit_baseline_path('chromium'),

This patch is doing two different things.
1. Allowing for an overrides expectations file.
2. Preparing to move test results upstream.

In the future, it would be easier to separate these out into separate patches. It makes each patch easier to review and makes debugging any regressions easier.

[Dirk Pranke]

(In reply to comment #2)
> I know you're just moving this code, but maybe move these into helper functions
> (e.g. logErrors() and addTestsExpectedToPass())? I'm more allergic to large
> blocks of code in constructors. 
> 

Yeah, I realized last night after I submitted this patch that I needed to do this, too. Done.

> 
> s/is is/it is
>

Done.
 
> >      def path_to_test_expectations_file(self):
> > -        return self.path_from_chromium_base('webkit', 'tools', 'layout_tests',
> > -                                            'test_expectations.txt')
> > +        return self.path_from_webkit_base('LayoutTests', 'platform',
> > +            'chromium', 'test_expectations.txt')
> 
> Did you mean to include this in this patch? Seems like this should go along
> with actually upstreaming the file.

I did mean to do this because I wanted to ensure that I could move the baselines and expectations
files without needing to change the driver scripts, but this should've been a separate CL (along w/ 
the changes in the baseline paths). I've rolled this out for now.
 
> This patch is doing two different things.
> 1. Allowing for an overrides expectations file.
> 2. Preparing to move test results upstream.
> 
> In the future, it would be easier to separate these out into separate patches.
> It makes each patch easier to review and makes debugging any regressions
> easier.

Agreed. I got lazy last night. I've split it out.

[Dirk Pranke]

Created attachment 51087 [details]
Revise w/ Ojan's feedback - don't change expectations and baseline paths.

[Dirk Pranke]

Created attachment 51088 [details]
Update Changelog

[Ojan Vafai]

Comment on attachment 51088 [details]
Update Changelog

LGTM

[Dimitri Glazkov (Google)]

Comment on attachment 51088 [details]
Update Changelog

rs=me.

[Dirk Pranke]

Committed r56193: <http://trac.webkit.org/changeset/56193>