Bug 96205 - TestExpectationsChecker._determine_port_from_expectations_path() does not support cascaded TestExpectations
Summary: TestExpectationsChecker._determine_port_from_expectations_path() does not sup...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 96204 96208
  Show dependency treegraph
 
Reported: 2012-09-09 06:36 PDT by Chris Dumez
Modified: 2012-09-10 15:21 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.95 KB, patch)
2012-09-09 06:39 PDT, Chris Dumez
tony: review-
tony: commit-queue-
Details | Formatted Diff | Diff
Patch (4.23 KB, patch)
2012-09-10 12:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-09-09 06:36:25 PDT
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
Comment 1 Chris Dumez 2012-09-09 06:39:56 PDT
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
Comment 2 Tony Chang 2012-09-10 09:54:42 PDT
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.
Comment 3 Chris Dumez 2012-09-10 11:02:45 PDT
(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.
Comment 4 Tony Chang 2012-09-10 11:24:06 PDT
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.
Comment 5 Chris Dumez 2012-09-10 12:09:17 PDT
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)
Comment 6 WebKit Review Bot 2012-09-10 15:21:43 PDT
Comment on attachment 163178 [details]
Patch

Clearing flags on attachment: 163178

Committed r128117: <http://trac.webkit.org/changeset/128117>
Comment 7 WebKit Review Bot 2012-09-10 15:21:47 PDT
All reviewed patches have been landed.  Closing bug.