Bug 200717 - W3C test importer should be able to handle expected references with an absolute path.
Summary: W3C test importer should be able to handle expected references with an absolu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Carlos Alberto Lopez Perez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-14 08:32 PDT by Carlos Alberto Lopez Perez
Modified: 2019-08-27 05:32 PDT (History)
11 users (show)

See Also:


Attachments
Patch (11.61 KB, patch)
2019-08-15 05:46 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (11.66 KB, patch)
2019-08-15 05:50 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (11.33 KB, patch)
2019-08-26 10:30 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (13.07 KB, patch)
2019-08-26 17:36 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2019-08-14 08:32:25 PDT
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.
Comment 1 Carlos Alberto Lopez Perez 2019-08-15 05:46:11 PDT
Created attachment 376373 [details]
Patch
Comment 2 Carlos Alberto Lopez Perez 2019-08-15 05:50:13 PDT
Created attachment 376374 [details]
Patch
Comment 3 youenn fablet 2019-08-23 07:49:07 PDT
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?
Comment 4 Carlos Alberto Lopez Perez 2019-08-26 10:02:02 PDT
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.
Comment 5 Carlos Alberto Lopez Perez 2019-08-26 10:30:32 PDT
Created attachment 377253 [details]
Patch

patch v2: pass to the test parser the right source root directory instead of a list of directories
Comment 6 youenn fablet 2019-08-26 10:35:41 PDT
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"?
Comment 7 Carlos Alberto Lopez Perez 2019-08-26 17:18:24 PDT
(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
Comment 8 Carlos Alberto Lopez Perez 2019-08-26 17:36:19 PDT
Created attachment 377300 [details]
Patch

Patch for landing
Comment 9 WebKit Commit Bot 2019-08-27 05:31:57 PDT
Comment on attachment 377300 [details]
Patch

Clearing flags on attachment: 377300

Committed r249139: <https://trac.webkit.org/changeset/249139>
Comment 10 WebKit Commit Bot 2019-08-27 05:31:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-08-27 05:32:18 PDT
<rdar://problem/54744672>