WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WORKSFORME
83048
new-webkit-run-tests: handle ref tests from the CSS test suite
https://bugs.webkit.org/show_bug.cgi?id=83048
Summary
new-webkit-run-tests: handle ref tests from the CSS test suite
Robert Hogan
Reported
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
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Robert Hogan
Comment 1
2012-04-03 12:21:32 PDT
Created
attachment 135390
[details]
Patch
Ryosuke Niwa
Comment 2
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?
Robert Hogan
Comment 3
2012-04-03 12:49:05 PDT
Comment on
attachment 135390
[details]
Patch More to be done here.
Dirk Pranke
Comment 4
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.
Robert Hogan
Comment 5
2012-04-08 16:28:52 PDT
Created
attachment 136161
[details]
Patch
Robert Hogan
Comment 6
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.
Robert Hogan
Comment 7
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.
Ryosuke Niwa
Comment 8
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.
Robert Hogan
Comment 9
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.
Ryosuke Niwa
Comment 10
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.
Robert Hogan
Comment 11
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.
Ryosuke Niwa
Comment 12
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.
Dirk Pranke
Comment 13
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.
Robert Hogan
Comment 14
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.
Robert Hogan
Comment 15
2012-04-21 08:22:22 PDT
Created
attachment 138234
[details]
Patch
Ryosuke Niwa
Comment 16
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.
Dirk Pranke
Comment 17
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.
Robert Hogan
Comment 18
2012-04-25 11:04:39 PDT
Created
attachment 138846
[details]
Patch
Ryosuke Niwa
Comment 19
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.
Robert Hogan
Comment 20
2012-04-26 12:22:17 PDT
Committed
r115340
: <
http://trac.webkit.org/changeset/115340
>
Dimitri Glazkov (Google)
Comment 21
2012-04-26 13:46:48 PDT
Reverted
r115340
for reason: Does not work with Windows. Committed
r115352
: <
http://trac.webkit.org/changeset/115352
>
Dirk Pranke
Comment 22
2012-07-17 13:40:47 PDT
i will take a look
Robert Hogan
Comment 23
2012-07-17 13:52:07 PDT
Created
attachment 152818
[details]
Patch
Eric Seidel (no email)
Comment 24
2012-08-22 15:40:07 PDT
Looks like this one got forgotten.
Dirk Pranke
Comment 25
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 ...
Sam Sneddon [:gsnedders]
Comment 26
2021-04-30 18:45:32 PDT
we've been running them for years at this point
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug