Bug 137586 – import-w3c-test rewrites relative src paths in ref files incorrectly

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Rebecca Hauck]

There is a bug where <script src="../../../foo.js"></script> gets rewritten as "../../foo.js"></script> when its relative path is updated.

The offending LOC is Line 175 in test_converter.py

[Rebecca Hauck]

Created attachment 239587 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 239587 [details]
Patch

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

> Tools/ChangeLog:7
> +        This patch fixes a bug in test_converter.py where src paths weren't
> +        getting rewritten properly. It also adds some tests for this case.

i think you need to write a better description, it looks like this patch not only fixes src path rewriting, but also adds support for rewriting source paths in style elements.

> Tools/Scripts/webkitpy/w3c/test_converter.py:46
> +

Nit: This newline is just noise in the diff.

> Tools/Scripts/webkitpy/w3c/test_converter.py:154
> +        return self.convert_reference_relpaths(converted[1])
> +

it looks like you accidentally duplicated this return statement?

> Tools/Scripts/webkitpy/w3c/test_converter.py:175
> -                    converted = self.convert_reference_relpaths(attr_value)
> +                    converted = re.sub(attr_value, self.convert_reference_relpaths(attr_value), converted)

I don't think I understand how this fixed the issue. Can you explain?

> Tools/Scripts/webkitpy/w3c/test_importer.py:279
> +

Nit: This looks line another stray line.

[Rebecca Hauck]

Created attachment 239821 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 239821 [details]
Patch

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

You need to rebase the patch, it no longer applies. Also, see below.

> Tools/Scripts/webkitpy/w3c/test_converter.py:170
> -            for attr_name, attr_value in attrs:
> +            for attr in attrs:

Why the switch here? It seems much more readable to use atter_name and attr_value instead of attr[0] and attr[1].

> Tools/Scripts/webkitpy/w3c/test_converter.py:171
>                  if attr_name == 'src':

attr_name isn't defined.

[Rebecca Hauck]

Comment on attachment 239821 [details]
Patch

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

>> Tools/Scripts/webkitpy/w3c/test_converter.py:170
>> +            for attr in attrs:
> 
> Why the switch here? It seems much more readable to use atter_name and attr_value instead of attr[0] and attr[1].

I mentioned this in my previous patch. I changed this to be more consistent with the other blocks in this function.  If you think attr_name/value are better, I can change the other blocks in this function so they are all consistent.  what do you think?

>> Tools/Scripts/webkitpy/w3c/test_converter.py:171
>>                  if attr_name == 'src':
> 
> attr_name isn't defined.

oops, missed that. I'll fix in next patch.

[Bem Jones-Bey]

(In reply to comment #5)
> (From update of attachment 239821 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239821&action=review
> 
> >> Tools/Scripts/webkitpy/w3c/test_converter.py:170
> >> +            for attr in attrs:
> > 
> > Why the switch here? It seems much more readable to use atter_name and attr_value instead of attr[0] and attr[1].
> 
> I mentioned this in my previous patch. I changed this to be more consistent with the other blocks in this function.  If you think attr_name/value are better, I can change the other blocks in this function so they are all consistent.  what do you think?

I must have missed that. I think it's always better to have names than random indexes. Indexes are too easy to mess up and are much harder to read.

I think it would be better to fix the other blocks in a followup patch, just so that this one can stay focused on the problem at hand.

> 
> >> Tools/Scripts/webkitpy/w3c/test_converter.py:171
> >>                  if attr_name == 'src':
> > 
> > attr_name isn't defined.
> 
> oops, missed that. I'll fix in next patch.

I think you should stick with the names, so might as well leave it, but change how the iteration works. :-)

[Rebecca Hauck]

Created attachment 239898 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 239898 [details]
Patch

Unfortunately, now the test is broken:
[1029/1431] webkitpy.w3c.test_converter_unittest.W...Test.test_convert_attributes_if_needed failed:
  Traceback (most recent call last):
    File "/Users/bjonesbe/Code/webkit/svn/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py", line 318, in test_convert_attributes_if_needed
      self.verify_reference_relative_paths(converted, test_reference_support_info)
    File "/Users/bjonesbe/Code/webkit/svn/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py", line 358, in verify_reference_relative_paths
      self.assertTrue(expected_tag in converted[1], 'relative path ' + path + ' was not converted correcty')
  AssertionError: relative path ../../some-script.js was not converted correcty

[Rebecca Hauck]

Created attachment 239903 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 239903 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 239903 [details]
Patch

Clearing flags on attachment: 239903

Committed r174748: <http://trac.webkit.org/changeset/174748>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.