W3C test importer should be able to handle expected references with an absolute path.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

This happened when trying to use the tool to import the WPT css/css-images tests in bug 200716

Some WPT tests, like css/css-images/multiple-position-color-stop-linear-2.html contain an absolute path to the testref instead of a relative one.

$ grep rel=match ${WPTBASEDIR}/css/css-images/multiple-position-color-stop-linear-2.html
<link rel=match href=/css/css-images/multiple-position-color-stop-linear-2-ref.html>


This is OK for WPT because the tests are not ran directly from the filesystem, but from a webserver where the absolute path simply points to ${WPTBASEDIR}.

But this confuses our import tool, and causes that it is unable to find the right file.

[Carlos Alberto Lopez Perez]

Created attachment 376373 [details]
Patch

[Carlos Alberto Lopez Perez]

Created attachment 376374 [details]
Patch

[youenn fablet]

Comment on attachment 376374 [details]
Patch

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

> Tools/ChangeLog:17
> +        web-platform-tests/css/css-images/multiple-position-color-stop-linear-2.html

In that particular case, the test would be simpler with the path being relative.
Doing a small grep, there are like 5 tests that seem to have absolute paths.
Maybe we should mandate wpt tests to have relative paths?

> Tools/Scripts/webkitpy/w3c/test_parser.py:96
> +                    for source_root_directory in self.source_root_directories:

It seems a bit odd to iterate through all directories.
There should be only one root directory in fact.
Can the patch be refactored accordingly?

[Carlos Alberto Lopez Perez]

Comment on attachment 376374 [details]
Patch

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

>> Tools/ChangeLog:17
>> +        web-platform-tests/css/css-images/multiple-position-color-stop-linear-2.html
> 
> In that particular case, the test would be simpler with the path being relative.
> Doing a small grep, there are like 5 tests that seem to have absolute paths.
> Maybe we should mandate wpt tests to have relative paths?

That is something that should be discussed with the WPT project itself.
And even if they agree to change that paths to relative ones, I think also should be done to prevent this from happening again like adding a check on their linter.

In any case, my intention here is to improve the WebKit synchronization script to work better and be able to handle this case of test references with absolute paths.

>> Tools/Scripts/webkitpy/w3c/test_parser.py:96
>> +                    for source_root_directory in self.source_root_directories:
> 
> It seems a bit odd to iterate through all directories.
> There should be only one root directory in fact.
> Can the patch be refactored accordingly?

Ok.

[Carlos Alberto Lopez Perez]

Created attachment 377253 [details]
Patch

patch v2: pass to the test parser the right source root directory instead of a list of directories

[youenn fablet]

Comment on attachment 377253 [details]
Patch

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

> Tools/Scripts/webkitpy/w3c/test_importer.py:313
> +                test_parser = TestParser(vars(self.options), filename=fullpath, host=self.host, source_root_directory=self._source_root_directory_for_file(fullpath))

Do we need to compute the root directory for every path?
Is source_root_directory equal to the directory path given to find_importable_tests?

> Tools/Scripts/webkitpy/w3c/test_parser.py:95
> +                if not self.filesystem.exists(ref_file) and href_match_file.startswith('/'):

The self.filesystem.exists(ref_file) check is probably not needed.
We probably want the href value to be whitespace-trimmed, is it the case?
Maybe we should add a test case where href="     /test"?

[Carlos Alberto Lopez Perez]

(In reply to youenn fablet from comment #6)
> Comment on attachment 377253 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377253&action=review
> 
> > Tools/Scripts/webkitpy/w3c/test_importer.py:313
> > +                test_parser = TestParser(vars(self.options), filename=fullpath, host=self.host, source_root_directory=self._source_root_directory_for_file(fullpath))
> 
> Do we need to compute the root directory for every path?
> Is source_root_directory equal to the directory path given to
> find_importable_tests?
> 

Its not the same, the directory that find_importable_tests() receives may contain test subpaths and can't be used directly as source_root_directory.
But is true that there is no need to calculate source_root_directory for every test processed, as it will be common for all of the tests inside a directory...  so it can be calculated only once per import.

> > Tools/Scripts/webkitpy/w3c/test_parser.py:95
> > +                if not self.filesystem.exists(ref_file) and href_match_file.startswith('/'):
> 
> The self.filesystem.exists(ref_file) check is probably not needed.
> We probably want the href value to be whitespace-trimmed, is it the case?
> Maybe we should add a test case where href="     /test"?

Good idea.

Thanks

[Carlos Alberto Lopez Perez]

Created attachment 377300 [details]
Patch

Patch for landing

[WebKit Commit Bot]

Comment on attachment 377300 [details]
Patch

Clearing flags on attachment: 377300

Committed r249139: <https://trac.webkit.org/changeset/249139>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54744672>