Bug 96205 – TestExpectationsChecker._determine_port_from_expectations_path() does not support cascaded TestExpectations

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Dumez]

TestExpectationsChecker._determine_port_from_expectations_path() currently uses port.path_to_test_expectations_file() instead of port.expectations_files(), which means that it considers only 1 TestExpectations file per port. This leads to problems for ports (such as EFL) that support cascaded TestExpectations.

This leads to errors such as this one with the style checker:
LayoutTests/platform/efl-wk1/TestExpectations:1:  No port uses path LayoutTests/platform/efl-wk1/TestExpectations for test_expectations  [test/expectations] [5]
Total errors found: 1 in 2 files

[Chris Dumez]

Created attachment 162994 [details]
Patch

Does not cause any regression:

$ ./Tools/Scripts/test-webkitpy
Suppressing most webkitpy logging while running unit tests.
Skipping tests in the following modules or packages because they are really, really, slow:
    webkitpy.common.checkout.scm.scm_unittest
    (https://bugs.webkit.org/show_bug.cgi?id=31818; use --all to include)

Ran 1615 tests in 11.699s                                                                                                                                   

OK

[Tony Chang]

Comment on attachment 162994 [details]
Patch

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

> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:57
> +            for test_expectation_file in port.expectations_files():
> +                if test_expectation_file.replace(port.path_from_webkit_base() + host.filesystem.sep, '') == expectations_path:
> +                    return port

I don't see how this change causes us to consider more TestExpectations files per port.  It looks like this is just causing us to use a different port.

Is the problem that you're editing efl-wk2/TestExpectations and it's not checking the file (because it's only looking at efl/TestExpectations)?  It would be clearer if you added a unittest demonstrating the problem.

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 162994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162994&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/test_expectations.py:57
> > +            for test_expectation_file in port.expectations_files():
> > +                if test_expectation_file.replace(port.path_from_webkit_base() + host.filesystem.sep, '') == expectations_path:
> > +                    return port
> 
> I don't see how this change causes us to consider more TestExpectations files per port.  It looks like this is just causing us to use a different port.
> 
> Is the problem that you're editing efl-wk2/TestExpectations and it's not checking the file (because it's only looking at efl/TestExpectations)?  It would be clearer if you added a unittest demonstrating the problem.

The problem is that, whenever we add a test to efl-wk2/TestExpectations or efl-wk1/TestExpectations, the style checker will complain that those TestExpectations files are not used by any port. This is quite annoying as the cq will reject our gardening patches and we currently need to commit them manually.

I don't understand your comment frankly. I don't see why my change could cause us to use a different port. Basically, instead of considering the output of port.path_to_test_expectations_file() for each port, I iterate over the list returned by port.expectations_files(). The default implementation of port.expectations_files() will return [path_to_test_expectations_file(),] so this won't change anything for ports using the default implementation. However, for ports such as EFL that redefine expectations_files(), it will cause the function to properly recognize efl-wk2/TestExpectations or efl-wk1/TestExpectations as belonging to EFL port.

[Tony Chang]

Comment on attachment 162994 [details]
Patch

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 162994 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=162994&action=review
> > 
> > > Tools/Scripts/webkitpy/style/checkers/test_expectations.py:57
> > > +            for test_expectation_file in port.expectations_files():
> > > +                if test_expectation_file.replace(port.path_from_webkit_base() + host.filesystem.sep, '') == expectations_path:
> > > +                    return port
> > 
> > I don't see how this change causes us to consider more TestExpectations files per port.  It looks like this is just causing us to use a different port.
> > 
> > Is the problem that you're editing efl-wk2/TestExpectations and it's not checking the file (because it's only looking at efl/TestExpectations)?  It would be clearer if you added a unittest demonstrating the problem.
> 
> The problem is that, whenever we add a test to efl-wk2/TestExpectations or efl-wk1/TestExpectations, the style checker will complain that those TestExpectations files are not used by any port.

Thank you, this is what I didn't understand.  I thought you were saying the problem was you were looking at the wrong TestExpectations file, rather than being unable to find a port.

> However, for ports such as EFL that redefine expectations_files(), it will cause the function to properly recognize efl-wk2/TestExpectations or efl-wk1/TestExpectations as belonging to EFL port.

I would include this in your ChangeLog description.

The change looks correct to me, but you need a unittest.

[Chris Dumez]

Created attachment 163178 [details]
Patch

Take Tony Chang's feedback into consideration.

Note that I also had to construct the port twice with "webkit_test_runner" option set to True and False in order to retrieve the TestExpectations paths for both WebKit1 and WebKit2.

For example, the EFL port returns
["efl-wk1/TestExpectations", "efl/TestExpectations"] for WebKit1 ("webkit_test_runner" option is False)
["efl-wk2/TestExpectations", "efl/TestExpectations"] for WebKit2 ("webkit_test_runner" option is True)

[WebKit Review Bot]

Comment on attachment 163178 [details]
Patch

Clearing flags on attachment: 163178

Committed r128117: <http://trac.webkit.org/changeset/128117>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.