Bug 200717

Summary: W3C test importer should be able to handle expected references with an absolute path.
Product: WebKit Reporter: Carlos Alberto Lopez Perez <clopez>
Component: Tools / TestsAssignee: Carlos Alberto Lopez Perez <clopez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, cgarcia, commit-queue, ews-watchlist, fred.wang, glenn, jbedard, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Carlos Alberto Lopez Perez
Reported 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.
Attachments
Patch (11.61 KB, patch)
2019-08-15 05:46 PDT, Carlos Alberto Lopez Perez 		no flags
DetailsFormatted DiffDiff
Patch (11.66 KB, patch)
2019-08-15 05:50 PDT, Carlos Alberto Lopez Perez 		no flags
DetailsFormatted DiffDiff
Patch (11.33 KB, patch)
2019-08-26 10:30 PDT, Carlos Alberto Lopez Perez 		no flags
DetailsFormatted DiffDiff
Patch (13.07 KB, patch)
2019-08-26 17:36 PDT, Carlos Alberto Lopez Perez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Carlos Alberto Lopez Perez
Comment 1 2019-08-15 05:46:11 PDT
Created attachment 376373 [details] Patch
Carlos Alberto Lopez Perez
Comment 2 2019-08-15 05:50:13 PDT
Created attachment 376374 [details] Patch
youenn fablet
Comment 3 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?
Carlos Alberto Lopez Perez
Comment 4 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.
Carlos Alberto Lopez Perez
Comment 5 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
youenn fablet
Comment 6 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"?
Carlos Alberto Lopez Perez
Comment 7 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
Carlos Alberto Lopez Perez
Comment 8 2019-08-26 17:36:19 PDT
Created attachment 377300 [details] Patch Patch for landing
WebKit Commit Bot
Comment 9 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>
WebKit Commit Bot
Comment 10 2019-08-27 05:31:59 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 11 2019-08-27 05:32:18 PDT
<rdar://problem/54744672>
Note You need to log in before you can comment on or make changes to this bug.