[Webkit-unassigned] [Bug 135929] import-w3c-tests doesn't handle relative paths to support files in ref files correctly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135929


Bem Jones-Bey <bjonesbe at adobe.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #237703|review?                     |review-
               Flag|                            |




--- Comment #6 from Bem Jones-Bey <bjonesbe at adobe.com>  2014-09-08 11:09:13 PST ---
(From update of attachment 237703)
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}

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.