[webkit-reviews] review denied: [Bug 135929] import-w3c-tests doesn't handle relative paths to support files in ref files correctly : [Attachment 237703] Patch

[bugzilla-daemon at webkit.org]

Bem Jones-Bey <bjonesbe at adobe.com> has denied Rebecca Hauck
<rhauck at adobe.com>'s request for review:
Bug 135929: import-w3c-tests doesn't handle relative paths to support files in
ref files correctly
https://bugs.webkit.org/show_bug.cgi?id=135929

Attachment 237703: Patch
https://bugs.webkit.org/attachment.cgi?id=237703&action=review

------- Additional Comments from Bem Jones-Bey <bjonesbe at adobe.com>
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}