Summary: | import-w3c-test rewrites relative src paths in ref files incorrectly | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rebecca Hauck <rhauck> | ||||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bjonesbe, commit-queue, glenn | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Rebecca Hauck
2014-10-09 17:45:22 PDT
Created attachment 239587 [details]
Patch
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. Created attachment 239821 [details]
Patch
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. 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. (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. :-) Created attachment 239898 [details]
Patch
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
Created attachment 239903 [details]
Patch
Comment on attachment 239903 [details]
Patch
r=me
Comment on attachment 239903 [details] Patch Clearing flags on attachment: 239903 Committed r174748: <http://trac.webkit.org/changeset/174748> All reviewed patches have been landed. Closing bug. |