[webkit-reviews] review denied: [Bug 96205] TestExpectationsChecker._determine_port_from_expectations_path() does not support cascaded TestExpectations : [Attachment 162994] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Christophe Dumez
<christophe.dumez at intel.com>'s request for review:
Bug 96205: TestExpectationsChecker._determine_port_from_expectations_path()
does not support cascaded TestExpectations
https://bugs.webkit.org/show_bug.cgi?id=96205

Attachment 162994: Patch
https://bugs.webkit.org/attachment.cgi?id=162994&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
(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.