Bug 135929 – import-w3c-tests doesn't handle relative paths to support files in ref files correctly

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Rebecca Hauck]

I discovered this when importing the W3C CSS Shapes test suite, so it's easy to repro by just re-importing that.

Most of the W3C tests live in reference/ subdirectory and they are imported into the same directory as the test with the -expected.html suffix to conform to WebKit's conventions. When they contain relative paths to support files (images, css files, scripts), those path must be updated so they're relative to the new location. This was working at one point, but must have broken somewhere.

[Rebecca Hauck]

Created attachment 237605 [details]
Patch

[WebKit Commit Bot]

Attachment 237605 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Bem Jones-Bey]

Comment on attachment 237605 [details]
Patch

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

> Tools/ChangeLog:8
> +	The recent refactor of the W3C test repo falsified a bunch of assmumptions that

Fix the indentation here. (The style bot also complained. :-)

> Tools/Scripts/webkitpy/w3c/test_converter.py:40
> +def convert_for_webkit(new_path, filename, refrelpath, host=Host()):

Nit: I don't really like the name refrelpath. I think for consistency with resources_relpath below, this should at least be ref_relpath. reference_relpath would be better, I think. (I'd really like refernce_relative_path, but relpath is used in a bunch of places already, so probably best to stick with that for now.) It's up to you if you change it.

> Tools/Scripts/webkitpy/w3c/test_converter.py:174
> +                if attr_name == 'src' and  not attr_value.startswith('data:'):

Do you ever want to replace if you have a URL scheme at all? For example, if you have http: or file:, you probably don't want to do the replacement, either.

Also, shouldn't you be checking the scheme in the case of a url() in convert_relative_reffile_paths()? It seems like a helper function to actually do the replacement (or not if you have a path that doesn't need a replacement) would be a good idea.

> Tools/Scripts/webkitpy/w3c/test_parser.py:103
> +                    test_info['refrelpath'] = self.filesystem.relpath(self.filename, ref_file).replace('../', '', 1).replace(testfile[1], '')

Why do you remove the first '../'?

> Tools/Scripts/webkitpy/w3c/test_parser.py:-137
> -        """ Searches the file for all paths specified in url()'s, href or src attributes."""

Why did you remove the search for href attributes?

[Rebecca Hauck]

Created attachment 237703 [details]
Patch

[Rebecca Hauck]

Comment on attachment 237605 [details]
Patch

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

>> Tools/ChangeLog:8
>> +	The recent refactor of the W3C test repo falsified a bunch of assmumptions that
> 
> Fix the indentation here. (The style bot also complained. :-)

Fixed in new patch.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:40
>> +def convert_for_webkit(new_path, filename, refrelpath, host=Host()):
> 
> Nit: I don't really like the name refrelpath. I think for consistency with resources_relpath below, this should at least be ref_relpath. reference_relpath would be better, I think. (I'd really like refernce_relative_path, but relpath is used in a bunch of places already, so probably best to stick with that for now.) It's up to you if you change it.

In the new patch I've changed this to be reference_relpath to be consistent with resources_relpath. I'm also now passing in a reference_support dictionary here with 2 keys- reference_relpath and reference_support_files to the replacement code will replace them specifically rather than using regex's.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:174
>> +                if attr_name == 'src' and  not attr_value.startswith('data:'):
> 
> Do you ever want to replace if you have a URL scheme at all? For example, if you have http: or file:, you probably don't want to do the replacement, either.
> 
> Also, shouldn't you be checking the scheme in the case of a url() in convert_relative_reffile_paths()? It seems like a helper function to actually do the replacement (or not if you have a path that doesn't need a replacement) would be a good idea.

In the new patch, the converter gets the full list of ref support paths, so instead of this check, I check if the attr_value is any of the support file paths. Similar thing in convert_relative_reffile_paths. Also, I've expanded the list of tags that can have src attributes. May be overkill, but it's harmless.

>> Tools/Scripts/webkitpy/w3c/test_parser.py:103
>> +                    test_info['refrelpath'] = self.filesystem.relpath(self.filename, ref_file).replace('../', '', 1).replace(testfile[1], '')
> 
> Why do you remove the first '../'?

Because filesystem.relpath (which is os.relpath) assumes that its arguments are directories. In the new patch, I've changed the call to relpath to be on each file's parent dir to avoid this confusion.

>> Tools/Scripts/webkitpy/w3c/test_parser.py:-137
>> -        """ Searches the file for all paths specified in url()'s, href or src attributes."""
> 
> Why did you remove the search for href attributes?

I assumed that hrefs are only in metadata and the only place where there's an actual path to a file in metadata is for reference files, which are retrieved and handled specifically through their rel=match attribute. I guess there could be a path in an a href though, so I put this back in the new patch.

[Bem Jones-Bey]

Comment on attachment 237703 [details]
Patch

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

I like this structure better than the first one, but I think you have a bug in here with using an undefined variable (and then lots of naming and formatting comments)

> Tools/ChangeLog:9
> +        were made when this script was originally writted re: relative paths in ref files.

s/writted/written
Nit: s/re:/with respect to/

> Tools/ChangeLog:10
> +        This patch updates paths in ref files if they move relative to the test file.       

Nit: This patch updates import-w3c-tests to update paths in ref files if they move relative to the test file.

> Tools/Scripts/webkitpy/w3c/test_converter.py:40
> +def convert_for_webkit(new_path, filename, reference_support, host=Host()):

This needs a better name than "reference_support", I'm really not sure what the variable is for or what it contains from the name.

> Tools/Scripts/webkitpy/w3c/test_converter.py:133
> +        for path in self.reference_support['reference_support_files']:

Why not just call this "files" instead of "reference_support_files"?

> Tools/Scripts/webkitpy/w3c/test_converter.py:136
> +                # FIXME: See comment convert_attributes_if_needed re: WebKit bug #136577. Same applies here.
> +                new_path = re.sub(self.reference_support['reference_relpath'], '', path, 1)

Instead of having to point to convert_attributes_if_needed, you should create a function to do the replacement and have one comment in that function.

eg: new_path = self.convert_support_file_path(path)

> Tools/Scripts/webkitpy/w3c/test_converter.py:139
> +        return converted

This variable is undefined here, since it's only mentioned in the nested if statement above.

> Tools/Scripts/webkitpy/w3c/test_converter.py:150
> +        if self.reference_support is not None and self.reference_support != {}:
> +            converted = self.convert_reference_relpaths(converted[1])
> +            return converted
> +        else:
> +            return converted[1]

All the negations are hard to read, so I'd do this the other way around, and webkit style says that you don't use an else when it isn't needed because of a return:

if self.reference_support is None or self.reference_support == {}:
    return converted[1]

return self.convert_reference_relpaths(converted[1])

> Tools/Scripts/webkitpy/w3c/test_converter.py:186
> +                    # FIXME: The following line of code assumes that the reference file originated
> +                    # in a subdirectory of the test file's parent directory and that it is being
> +                    # imported up N levels to be the same directory as the test. Hence, we're
> +                    # removing the relpath from the attr_value here. This won't work if the
> +                    # reference file originated in a directory above the test file and is imported
> +                    # down N levels. This scenario occurs many times in the CSS test repo, but
> +                    # it happens to be one single shared reference file that many of the css21
> +                    # tests share from a common ancestor directory. In all of these cases,
> +                    # the original ref file and the test file's parent dir both have the
> +                    # same relative path to the ref file's support file, so the new location
> +                    # of the ref file is a lateral move and everything 'just works'.  Ideally,
> +                    # the import script should do the right thing wherever the the ref file
> +                    # originates, but this is an edge case and highly unlikely.
> +                    # Logged as Webkit bug #135677.
> +                    new_path = re.sub(self.reference_support['reference_relpath'], '', attr_value, 1)

I'd move the comment and re.sub to it's own function and call that here. Eg: new_path = self.convert_support_file_path(attr_value)

Also in the comment, instead of saying "Logged as WebKit bug #135677", put a URL to the tracker, like http://webkit.org/b/135677

This is more of a nit, but I think you could make the comment shorter, and just have the full explanation in the bug, since most readers of the code won't need the full explanation. Maybe something like "FIXME: This doesn't handle an edge case where simply removing the relative path doesn't work. See http://webkit.org/b/135677 for details."

> Tools/Scripts/webkitpy/w3c/test_parser.py:102
> +                    test_info['reference_support'] = {'reference_relpath': reference_relpath, 'reference_support_files': reference_support_files}

I'd rename this like so:
test_info['reference_support_info'] = {'reference_relpath': reference_relpath, 'files': reference_support_files}

[Rebecca Hauck]

Created attachment 237877 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 237877 [details]
Patch

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

r=me, but please make the following updates before landing

> Tools/Scripts/webkitpy/w3c/test_converter.py:149
> +        else:

WebKit style is to omit this else.

> Tools/Scripts/webkitpy/w3c/test_converter.py:172
> +                if attr_name == 'src' and attr_value in self.reference_support_info['files']:
> +                    converted = self.convert_reference_relpaths(attr_value)

You're walking through reference_support_info['files'] twice in this situation. Either remove the check here ("and attr_value in self.reference_support_info['files']"), or create a more specific helper function to do the replacement, and call that both from here and from convert_reference_relpaths.

> Tools/Scripts/webkitpy/w3c/test_parser.py:141
> +            if not(path.startswith('http:')) and not(path.startswith('mailto:')) and not(path.startswith('data:')):

This is fragile, I think it would be better to do:

uri_scheme_pattern = re.compile(r"[A-Za-z][A-Za-z+.-]*:");
if uri_scheme_pattern.match(path):

I got the pattern from: http://tools.ietf.org/html/rfc3986#section-3.1
It will also match paths that start with a windows drive letter, but I'm pretty sure that such platform specific paths won't end up in the W3C repos. :-)

[Rebecca Hauck]

Created attachment 237914 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 237914 [details]
Patch

You didn't put my name in the ChangeLog as reviewed by, so I'm r+'ing again so that the CQ will do it.

[WebKit Commit Bot]

Comment on attachment 237914 [details]
Patch

Clearing flags on attachment: 237914

Committed r173498: <http://trac.webkit.org/changeset/173498>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This patch broke the "test-webkitpy" phase.

[1300/1433] webkitpy.w3c.test_converter_unittest.W3CTestConverterTest.test_convert_for_webkit_harness_only erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py", line 103, in test_convert_for_webkit_harness_only
      converter = _W3CTestConverter(fake_dir_path, DUMMY_FILENAME)
  TypeError: __init__() takes at least 4 arguments (3 given)
  
[1304/1433] webkitpy.w3c.test_converter_unittest.W3CTestConverterTest.test_convert_prefixed_properties erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py", line 271, in test_convert_prefixed_properties
      converter = _W3CTestConverter(DUMMY_PATH, DUMMY_FILENAME)
  TypeError: __init__() takes at least 4 arguments (3 given)
  
[1308/1433] webkitpy.w3c.test_converter_unittest.W3CTestConverterTest.test_convert_test_harness_paths erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py", line 194, in test_convert_test_harness_paths
      converter = _W3CTestConverter(fake_dir_path, DUMMY_FILENAME)
  TypeError: __init__() takes at least 4 arguments (3 given)
  
[1316/1433] webkitpy.w3c.test_parser_unittest.TestParserTest.test_analyze_jstest erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser_unittest.py", line 150, in test_analyze_jstest
      test_info = parser.analyze_test(test_contents=test_html)
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 77, in analyze_test
      matches = self.reference_links_of_type('match') + self.reference_links_of_type('mismatch')
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 112, in reference_links_of_type
      return self.test_doc.findAll(rel=reftest_type)
  AttributeError: 'NoneType' object has no attribute 'findAll'
  
[1320/1433] webkitpy.w3c.test_parser_unittest.TestParserTest.test_analyze_pixel_test_all_false erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser_unittest.py", line 208, in test_analyze_pixel_test_all_false
      test_info = parser.analyze_test(test_contents=test_html)
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 77, in analyze_test
      matches = self.reference_links_of_type('match') + self.reference_links_of_type('mismatch')
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 112, in reference_links_of_type
      return self.test_doc.findAll(rel=reftest_type)
  AttributeError: 'NoneType' object has no attribute 'findAll'
  
[1324/1433] webkitpy.w3c.test_parser_unittest.TestParserTest.test_analyze_pixel_test_all_true erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser_unittest.py", line 179, in test_analyze_pixel_test_all_true
      test_info = parser.analyze_test(test_contents=test_html)
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 77, in analyze_test
      matches = self.reference_links_of_type('match') + self.reference_links_of_type('mismatch')
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 112, in reference_links_of_type
      return self.test_doc.findAll(rel=reftest_type)
  AttributeError: 'NoneType' object has no attribute 'findAll'
  
[1328/1433] webkitpy.w3c.test_parser_unittest.TestParserTest.test_analyze_test_reftest_match_and_mismatch erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser_unittest.py", line 96, in test_analyze_test_reftest_match_and_mismatch
      test_info = parser.analyze_test(test_contents=test_html)
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 77, in analyze_test
      matches = self.reference_links_of_type('match') + self.reference_links_of_type('mismatch')
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 112, in reference_links_of_type
      return self.test_doc.findAll(rel=reftest_type)
  AttributeError: 'NoneType' object has no attribute 'findAll'
  
[1332/1433] webkitpy.w3c.test_parser_unittest.TestParserTest.test_analyze_test_reftest_multiple_matches erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser_unittest.py", line 70, in test_analyze_test_reftest_multiple_matches
      test_info = parser.analyze_test(test_contents=test_html)
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 77, in analyze_test
      matches = self.reference_links_of_type('match') + self.reference_links_of_type('mismatch')
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 112, in reference_links_of_type
      return self.test_doc.findAll(rel=reftest_type)
  AttributeError: 'NoneType' object has no attribute 'findAll'
  
[1336/1433] webkitpy.w3c.test_parser_unittest.TestParserTest.test_analyze_test_reftest_with_ref_support_Files erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser_unittest.py", line 130, in test_analyze_test_reftest_with_ref_support_Files
      test_info = parser.analyze_test(test_contents=test_html, ref_contents=ref_html)
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 77, in analyze_test
      matches = self.reference_links_of_type('match') + self.reference_links_of_type('mismatch')
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 112, in reference_links_of_type
      return self.test_doc.findAll(rel=reftest_type)
  AttributeError: 'NoneType' object has no attribute 'findAll'
  
[1421/1433] webkitpy.w3c.test_converter_unittest.W3CTestConverterTest.test_convert_for_webkit_harness_and_properties erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py", line 168, in test_convert_for_webkit_harness_and_properties
      converter = _W3CTestConverter(fake_dir_path, DUMMY_FILENAME)
  TypeError: __init__() takes at least 4 arguments (3 given)
  
[1422/1433] webkitpy.w3c.test_converter_unittest.W3CTestConverterTest.test_convert_for_webkit_nothing_to_convert erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py", line 81, in test_convert_for_webkit_nothing_to_convert
      converter = _W3CTestConverter(DUMMY_PATH, DUMMY_FILENAME)
  TypeError: __init__() takes at least 4 arguments (3 given)
  
[1423/1433] webkitpy.w3c.test_converter_unittest.W3CTestConverterTest.test_convert_for_webkit_properties_only erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py", line 131, in test_convert_for_webkit_properties_only
      converter = _W3CTestConverter(fake_dir_path, DUMMY_FILENAME)
  TypeError: __init__() takes at least 4 arguments (3 given)
  
[1424/1433] webkitpy.w3c.test_converter_unittest.W3CTestConverterTest.test_read_prefixed_property_list erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py", line 56, in test_read_prefixed_property_list
      converter = _W3CTestConverter(DUMMY_PATH, DUMMY_FILENAME)
  TypeError: __init__() takes at least 4 arguments (3 given)
  
[1427/1433] webkitpy.w3c.test_parser_unittest.TestParserTest.test_analyze_test_reftest_one_match erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser_unittest.py", line 49, in test_analyze_test_reftest_one_match
      test_info = parser.analyze_test(test_contents=test_html)
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 77, in analyze_test
      matches = self.reference_links_of_type('match') + self.reference_links_of_type('mismatch')
    File "/Volumes/Data/slave/mavericks-debug-tests-wk2/build/Tools/Scripts/webkitpy/w3c/test_parser.py", line 112, in reference_links_of_type
      return self.test_doc.findAll(rel=reftest_type)
  AttributeError: 'NoneType' object has no attribute 'findAll'
  
[1427/1433] webkitpy.common.system.executive_unittest.ExecutiveTest.serial_test_kill_process

[WebKit Commit Bot]

Re-opened since this is blocked by bug 136745

[Rebecca Hauck]

Created attachment 237971 [details]
Patch

[Bem Jones-Bey]

Bug 136752 should fix the tests.