Bug 83048 - new-webkit-run-tests: handle ref tests from the CSS test suite
Summary: new-webkit-run-tests: handle ref tests from the CSS test suite
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-03 11:11 PDT by Robert Hogan
Modified: 2017-07-18 08:30 PDT (History)
10 users (show)

See Also:


Attachments
Patch (15.70 KB, patch)
2012-04-03 12:21 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (1.89 MB, patch)
2012-04-08 16:28 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (1.89 MB, patch)
2012-04-21 08:22 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (1.88 MB, patch)
2012-04-25 11:04 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (1.83 MB, patch)
2012-07-17 13:52 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2012-04-03 11:11:09 PDT
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
Comment 1 Robert Hogan 2012-04-03 12:21:32 PDT
Created attachment 135390 [details]
Patch
Comment 2 Ryosuke Niwa 2012-04-03 12:42:15 PDT
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?
Comment 3 Robert Hogan 2012-04-03 12:49:05 PDT
Comment on attachment 135390 [details]
Patch

More to be done here.
Comment 4 Dirk Pranke 2012-04-03 16:16:29 PDT
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.
Comment 5 Robert Hogan 2012-04-08 16:28:52 PDT
Created attachment 136161 [details]
Patch
Comment 6 Robert Hogan 2012-04-09 03:25:24 PDT
(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.
Comment 7 Robert Hogan 2012-04-09 03:25:49 PDT
(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.
Comment 8 Ryosuke Niwa 2012-04-09 15:50:03 PDT
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.
Comment 9 Robert Hogan 2012-04-10 11:07:07 PDT
(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.
Comment 10 Ryosuke Niwa 2012-04-10 12:06:34 PDT
(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.
Comment 11 Robert Hogan 2012-04-10 13:08:35 PDT
(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.
Comment 12 Ryosuke Niwa 2012-04-10 13:15:30 PDT
(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.
Comment 13 Dirk Pranke 2012-04-10 15:51:56 PDT
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.
Comment 14 Robert Hogan 2012-04-21 08:15:11 PDT
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.
Comment 15 Robert Hogan 2012-04-21 08:22:22 PDT
Created attachment 138234 [details]
Patch
Comment 16 Ryosuke Niwa 2012-04-21 15:58:09 PDT
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.
Comment 17 Dirk Pranke 2012-04-23 13:00:38 PDT
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.
Comment 18 Robert Hogan 2012-04-25 11:04:39 PDT
Created attachment 138846 [details]
Patch
Comment 19 Ryosuke Niwa 2012-04-25 11:50:45 PDT
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.
Comment 20 Robert Hogan 2012-04-26 12:22:17 PDT
Committed r115340: <http://trac.webkit.org/changeset/115340>
Comment 21 Dimitri Glazkov (Google) 2012-04-26 13:46:48 PDT
Reverted r115340 for reason:

Does not work with Windows.

Committed r115352: <http://trac.webkit.org/changeset/115352>
Comment 22 Dirk Pranke 2012-07-17 13:40:47 PDT
i will take a look
Comment 23 Robert Hogan 2012-07-17 13:52:07 PDT
Created attachment 152818 [details]
Patch
Comment 24 Eric Seidel (no email) 2012-08-22 15:40:07 PDT
Looks like this one got forgotten.
Comment 25 Dirk Pranke 2012-08-22 17:52:03 PDT
didn't forget, just haven't been able to get to it yet :(. Still on my to-do list ...