Bug 83048 – new-webkit-run-tests: handle ref tests from the CSS test suite

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

[Robert Hogan]

These are stored in a ../reference folder linked from the test itself.

1. Need to create a reference folder in the layout_tests folder even though the tests in there are never run directly.
2. Handle comparing some/url to some/other/../url and recognizing they're the same.
3. Lots of bogus '%s is a reference test' output

[Robert Hogan]

Created attachment 135390 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 135390 [details]
Patch

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

> Tools/ChangeLog:19
> +        (ChromiumDriver.run_test): Treat some/test.html and some/other/../test.html as the same file.
> +        * Scripts/webkitpy/layout_tests/port/chromium_unittest.py:
> +        (ChromiumDriverTest.test_strip_uri): Test that some/test.html and some/other/../test.html are treated
> +         as the same file.

Why do we need this change? Don't non-Chromium ports need a similar change?

[Robert Hogan]

Comment on attachment 135390 [details]
Patch

More to be done here.

[Dirk Pranke]

Comment on attachment 135390 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:556
> +                uri = re.sub("[^/]+/\.\./", "", uri)

How is test_to_uri() returning a URI with '..' in it? That shouldn't be happening, and we probably need to fix that rather than work around it here.

[Robert Hogan]

Created attachment 136161 [details]
Patch

[Robert Hogan]

(In reply to comment #4)
> (From update of attachment 135390 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135390&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:556
> > +                uri = re.sub("[^/]+/\.\./", "", uri)
> 
> How is test_to_uri() returning a URI with '..' in it? That shouldn't be happening, and we probably need to fix that rather than work around it here.

This happens when the 'match' link in a test is something like:

<link rel="match" href="../reference/ref-nothing-below.htm">

See background-016.htm in the attached.

[Robert Hogan]

(In reply to comment #2)
> (From update of attachment 135390 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135390&action=review
> 
> > Tools/ChangeLog:19
> > +        (ChromiumDriver.run_test): Treat some/test.html and some/other/../test.html as the same file.
> > +        * Scripts/webkitpy/layout_tests/port/chromium_unittest.py:
> > +        (ChromiumDriverTest.test_strip_uri): Test that some/test.html and some/other/../test.html are treated
> > +         as the same file.
> 
> Why do we need this change? Don't non-Chromium ports need a similar change?

Other ports don't perform this test afaict.

[Ryosuke Niwa]

Comment on attachment 136161 [details]
Patch

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

> Tools/ChangeLog:22
> +        (Port.reference_files): Only return reference results in the reftest list that actually exist on
> +          disk. Also, don't ignore potential reference results on disk that are not in the reftest list, and
> +          don't ignore them just because we have a reftest list for the test's directory.

Oops, so there are reference files that are not listed on reftest.list? :(

I think Ojan specifically requested that this not be the case. It's very unfortunate...

I talked about this with Ojan briefly, and we both agreed that we either want this behavior only in imported test suites or fix imported reftest.list (preferred).
It seems like we can submit a patch to W3C test suite and then just apply that locally as well.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:556
> +                uri = re.sub("[^/]+/\.\./", "", uri)

This still doesn't make sense. Where are relative paths coming from? From reftest.list? If so, then we should just resolve those filenames in where we parse reftest.list.

[Robert Hogan]

(In reply to comment #8)
> (From update of attachment 136161 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136161&action=review
> 
> > Tools/ChangeLog:22
> > +        (Port.reference_files): Only return reference results in the reftest list that actually exist on
> > +          disk. Also, don't ignore potential reference results on disk that are not in the reftest list, and
> > +          don't ignore them just because we have a reftest list for the test's directory.
> 
> Oops, so there are reference files that are not listed on reftest.list? :(

Yes. Rather than add pixel results for tests from the CSS test suite we've added reference results where none were available from the suite itself.

> 
> I think Ojan specifically requested that this not be the case. It's very unfortunate...
> 
> I talked about this with Ojan briefly, and we both agreed that we either want this behavior only in imported test suites or fix imported reftest.list (preferred).
> It seems like we can submit a patch to W3C test suite and then just apply that locally as well.

The reftests (i.e. the result reference files) don't belong to the CSS test suite so we could remove them and let ports generate pixel results. That is something I'd like to avoid for now - it would be very useful to be able to add reftests for tests that don't yet have them in the CSS test suite.

Maybe a good alternative would be to move such tests to fast/css for now. That would keep css/2011323 as a 'clean' (though partial) import.


> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:556
> > +                uri = re.sub("[^/]+/\.\./", "", uri)
> 
> This still doesn't make sense. Where are relative paths coming from? From reftest.list? 

Yes. And also the 'match' link in the tests themselves.

>If so, then we should just resolve those filenames in where we parse reftest.list.

OK.

[Ryosuke Niwa]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 136161 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136161&action=review
> > 
> > > Tools/ChangeLog:22
> > > +        (Port.reference_files): Only return reference results in the reftest list that actually exist on
> > > +          disk. Also, don't ignore potential reference results on disk that are not in the reftest list, and
> > > +          don't ignore them just because we have a reftest list for the test's directory.
> > 
> > Oops, so there are reference files that are not listed on reftest.list? :(
> 
> Yes. Rather than add pixel results for tests from the CSS test suite we've added reference results where none were available from the suite itself.
...
> The reftests (i.e. the result reference files) don't belong to the CSS test suite so we could remove them and let ports generate pixel results. That is something I'd like to avoid for now - it would be very useful to be able to add reftests for tests that don't yet have them in the CSS test suite.

Oh, I don't think we want to do this. Read https://lists.webkit.org/pipermail/webkit-dev/2012-March/019782.html and replies.

> Maybe a good alternative would be to move such tests to fast/css for now. That would keep css/2011323 as a 'clean' (though partial) import.

Yes. We should.

> > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:556
> > > +                uri = re.sub("[^/]+/\.\./", "", uri)
> > 
> > This still doesn't make sense. Where are relative paths coming from? From reftest.list? 
> 
> Yes. And also the 'match' link in the tests themselves.

We should fix that in where we parse reftest.list.

[Robert Hogan]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 136161 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=136161&action=review
> > > 
> > > > Tools/ChangeLog:22
> > > > +        (Port.reference_files): Only return reference results in the reftest list that actually exist on
> > > > +          disk. Also, don't ignore potential reference results on disk that are not in the reftest list, and
> > > > +          don't ignore them just because we have a reftest list for the test's directory.
> > > 
> > > Oops, so there are reference files that are not listed on reftest.list? :(
> > 
> > Yes. Rather than add pixel results for tests from the CSS test suite we've added reference results where none were available from the suite itself.
> ...
> > The reftests (i.e. the result reference files) don't belong to the CSS test suite so we could remove them and let ports generate pixel results. That is something I'd like to avoid for now - it would be very useful to be able to add reftests for tests that don't yet have them in the CSS test suite.
> 
> Oh, I don't think we want to do this. Read https://lists.webkit.org/pipermail/webkit-dev/2012-March/019782.html and replies.
> 
> > Maybe a good alternative would be to move such tests to fast/css for now. That would keep css/2011323 as a 'clean' (though partial) import.
> 
> Yes. We should.

There are about 70 of them. We will want to remove them from fast/css when they eventually get imported to the css2.1 folder so does adding them to a folder called fast/css/css2.1-webkit sounds like the right idea? We can just put a note there to state that these are pending import to css2.1 when reftests are added.

[Ryosuke Niwa]

(In reply to comment #11)
> There are about 70 of them. We will want to remove them from fast/css when they eventually get imported to the css2.1 folder so does adding them to a folder called fast/css/css2.1-webkit sounds like the right idea? We can just put a note there to state that these are pending import to css2.1 when reftests are added.

I think this is a general problem we need to solve. We don't have good guidance on how to do this. Could you start a webkit-dev about it? We can also talk about it at contributor's meeting.

[Dirk Pranke]

Comment on attachment 136161 [details]
Patch

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

>>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:556
>>>> +                uri = re.sub("[^/]+/\.\./", "", uri)
>>> 
>>> This still doesn't make sense. Where are relative paths coming from? From reftest.list? If so, then we should just resolve those filenames in where we parse reftest.list.
>> 
>> Yes. And also the 'match' link in the tests themselves.
> 
> We should fix that in where we parse reftest.list.

I think this line is needed because we are handling relative paths to DRT in the first place; we shouldn't be doing that.  On line 533, above, driver_input.test_name should be a path relative to LayoutTests, and uri should be an absolute URL, not a relative URL. If that isn't true, we need to fix that.

This line should not be here.

[Robert Hogan]

Comment on attachment 136161 [details]
Patch

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

>>>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:556
>>>>> +                uri = re.sub("[^/]+/\.\./", "", uri)
>>>> 
>>>> This still doesn't make sense. Where are relative paths coming from? From reftest.list? If so, then we should just resolve those filenames in where we parse reftest.list.
>>> 
>>> Yes. And also the 'match' link in the tests themselves.
>> 
>> We should fix that in where we parse reftest.list.
> 
> I think this line is needed because we are handling relative paths to DRT in the first place; we shouldn't be doing that.  On line 533, above, driver_input.test_name should be a path relative to LayoutTests, and uri should be an absolute URL, not a relative URL. If that isn't true, we need to fix that.
> 
> This line should not be here.

What's happening is this:

driver_input.test_name: css2.1/20110323/../reference/ref-nothing-below.htm
uri: file:///home/robert/Development/WebKit/LayoutTests/css2.1/20110323/../reference/ref-nothing-below.htm
actual_uri: file:///home/robert/Development/WebKit/LayoutTests/css2.1/reference/ref-nothing-below.htm

So really this is specific to comparing actual_uri to uri. The only place these embedded relative paths are encountered is in reading reftest.list or in match expressions in the test itself. So I've fixed it in parsing reftest.list.

[Robert Hogan]

Created attachment 138234 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 138234 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/base.py:476
> +        for expectation, ref_test in reftest_list.get(self._filesystem.join(self.layout_tests_dir(), test_name), []):

There is abspath_for_test. You should use that.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:477
> +            ref_test = re.sub("[^/]+/\.\./", "", ref_test)

Can't we just use self._filesystem.abspath?

> LayoutTests/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description surrounded by blank lines.

[Dirk Pranke]

Comment on attachment 138234 [details]
Patch

As rniwa says, using abspath_for_test and self._filesystem.abspath() should get rid of the embedded ".."s. Otherwise the patch looks pretty good.

[Robert Hogan]

Created attachment 138846 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 138846 [details]
Patch

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

> Tools/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description.

> LayoutTests/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

This needs to appear before the description surrounded by blank lines.

[Robert Hogan]

Committed r115340: <http://trac.webkit.org/changeset/115340>

[Dimitri Glazkov (Google)]

Reverted r115340 for reason:

Does not work with Windows.

Committed r115352: <http://trac.webkit.org/changeset/115352>

[Dirk Pranke]

i will take a look

[Robert Hogan]

Created attachment 152818 [details]
Patch

[Eric Seidel (no email)]

Looks like this one got forgotten.

[Dirk Pranke]

didn't forget, just haven't been able to get to it yet :(. Still on my to-do list ...

[Sam Sneddon [:gsnedders]]

we've been running them for years at this point