Bug 111513 – Create a script to import W3C tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Rebecca Hauck]

We need an automated way to import W3C tests into WebKit. Both Webkit and the W3C support reftests, but they are in slightly different formats. The W3C defines the test:reference mappings using metadata in the test file while Webkit relies on a filenaming convention. Further, for CSS tests, many properties require the -webkit prefix and the W3C does not accept tests with prefixed properties.  We need a script to handle these conversions to make W3C tests compliant with the Webkit Way.

[Rebecca Hauck]

Created attachment 192007 [details]
Patch for the bug

[Rebecca Hauck]

Created attachment 192044 [details]
Fixed changelog - added newlines

[Rebecca Hauck]

The chromium tests failures here appear to be unrelated to my changes

[Ryosuke Niwa]

These scripts are missing BSD copyright headers. Also, can we somehow turn them into webkitpy modules like command or step?

[Ryosuke Niwa]

Turning them into webkitpy modules will allow us to easily test both scripts, which is of our interest if we're trying to automate the importation process.

[Dirk Pranke]

Rebecca, thanks for the scripts. They look like a great start.

It would be very useful to have examples of how to run these scripts somewhere, either here in the bug, on the wiki page, or in the ChangeLog.

There's at least two different classes of review feedback we can give here. The first is the simple feedback on the python style, to try and get this to mesh with the rest of the webkitpy. As Ryosuke points out, we need to add in the standard copyright headers, and it would be good to try and move much of this logic into modules so that we can write tests for them as appropriate.

The second, and much more important IMO is to work out some of the high-level steps and decisions over how we will actually import the tests. I would actually prefer to focus on that sort of feedback first, as there's not a lot of point in cleaning up the style until we are sure the scripts do what we want.

I'm still chewing through the patch (and want to play with it locally) so I don't have any of the second class of feedback yet, but will shortly.

[Rebecca Hauck]

(In reply to comment #5)
> Turning them into webkitpy modules will allow us to easily test both scripts, which is of our interest if we're trying to automate the importation process.

Ok, good point. And my apologies. I'm fairly new to Python :) I actually hadn't seen the Webkit Python guidelines until just now, so I can modify accordingly.

[Rebecca Hauck]

(In reply to comment #6)
> Rebecca, thanks for the scripts. They look like a great start.
> 
> It would be very useful to have examples of how to run these scripts somewhere, either here in the bug, on the wiki page, or in the ChangeLog.
> 
> There's at least two different classes of review feedback we can give here. The first is the simple feedback on the python style, to try and get this to mesh with the rest of the webkitpy. As Ryosuke points out, we need to add in the standard copyright headers, and it would be good to try and move much of this logic into modules so that we can write tests for them as appropriate.
> 
> The second, and much more important IMO is to work out some of the high-level steps and decisions over how we will actually import the tests. I would actually prefer to focus on that sort of feedback first, as there's not a lot of point in cleaning up the style until we are sure the scripts do what we want.
> 
> I'm still chewing through the patch (and want to play with it locally) so I don't have any of the second class of feedback yet, but will shortly.


Yeah I agree there are two tracks of feedback and also prefer to keep them second. I'm also more interested in the feedback on the import details.  I'm fairly new to Python, so in meantime, I'll read the WK Python guidelines so I'll be ready to make those changes too.

[Rebecca Hauck]

Created attachment 192111 [details]
one line fix for something silly

[Dirk Pranke]

Okay, so here's an initial round of feedback ...

1. r- at the very least to add the copyright info

2. as we discussed on #irc, I think we should merge these two scripts. I don't think that users necessarily should need to be aware of the need to generate manifest files.

3. ideally, this should work for anything in (at least) the CSSWG repo (by which I am referring to http://hg.csswg.org/test). I don't know if something broke in the patch you just uploaded, but it seems like I can't even run these scripts over the approved/css3-background dir that I could with the first patch. Should this work over the whole repo?

4. I think we'll need to get some sort of consensus on webkit-dev over how we want to import test suites before we land this and start importing things. If we have different people importing different sub directories into different parts of the tree, keeping things up to date could get very painful (we'd need to build a mapping of w3c directory to webkit directory somewhere).

I'm kinda thinking maybe we should try to put all of the tests in the entire csswg repo under LayoutTests/w3c/csswg or something.

Another potential option (with fairly obvious downsides) would be to actually clone or mirror the repo somewhere (either under LayoutTests/w3c or someplace outside of LayoutTests altogether).

Since these are kind of two parallel discussions, what I'd like to do is assume a strawman process for purposes of "importing" the tests such that we can run NRWT over them, but *not actually plan to check in imported tests yet*.

That way we can make progress on the import scripts and the code reviews while we debate the higher-level process.

Sound good? I'm open to other approaches, but I'm probably not going to be a fan of landing these scripts so that we can import individual directories in an ad-hoc / manual way in the mean time.

[Mihai Balan]

Comment on attachment 192111 [details]
one line fix for something silly

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

I also think that the two scripts should be merged. I can't quite see a scenario where I'd really want to use just one of the scripts. Sure, the separation allows avoiding some duplicate work but I think this could be done in the one-script scenario, too.

> Tools/Scripts/generate-w3c-testsuite-manifest:29
> +                    self.matches.append(attrs[i+1][1])

Are there any guarantees that the next attribute in the attrs list is the actual value we want to use?
Since the HTML tag does not force the order of a tag's attribute, there might be tests where the href attribute on a link element is not necessarily following the rel attribute, or where the rel attribute is the last one in the list (and thus doing attrs[i+1] will yield an IndexError)

Ditto for the next "if".

> Tools/Scripts/generate-w3c-testsuite-manifest:41
> +    print "prefixes in the test suite, open the manifest, uncomment prefixed.properties, and set its value"

Is the syntax of these manifest files documented somewhere?
I'd like these instructions to be a little more explicit - right now they seem to vague to me.

> Tools/Scripts/generate-w3c-testsuite-manifest:56
> +        testPath = sys.argv[1]

As there's no advanced processing of the parameters going on, I'd rather drop the testPath global variable and make the scanTestDir() method accept sys.argv[1] as a parameter.

> Tools/Scripts/generate-w3c-testsuite-manifest:59
> +

Don't leave empty lines between a block start (e.g. function def, 'for' start) and the block body (e.g. function body, 'for' body, etc.)

> Tools/Scripts/generate-w3c-testsuite-manifest:61
> +    ignoreFiles = ["-ref", "-notref", ".css", ".js", ".png", ".jpg", ".jpeg", ".DS_Store", ".mf", ".log"]

If these are just used as parameters to other functions, I'd rather have them as script constants/variables.

> Tools/Scripts/generate-w3c-testsuite-manifest:69
> +            if not shouldIgnore(filename, ignoreFiles):

(It's more a matter of personal taste but) I think methods should be defined before using them (re: shouldIgnore(), findReferences(), writeManifest() )

> Tools/Scripts/generate-w3c-testsuite-manifest:74
> +                #    print "No reference files found in " + filename

Why is this commented out? I think it's useful info.
Also, if this should be gone, the above if is useless ( list.extend() will work just as well with an empty list).

> Tools/Scripts/generate-w3c-testsuite-manifest:79
> +        for dirname in dirs:

I might be missing something obvious, but does your script recurse through directories?
If it doesn't, I don't think you need this piece of code anymore.

> Tools/Scripts/generate-w3c-testsuite-manifest:95
> +    #print "Reading " + testFile

A nice to have would be to enable this kind of progress message via a -v/--verbose flag passed to the script.

> Tools/Scripts/generate-w3c-testsuite-manifest:120
> +    # TODO Should I add a flag to make automatically overwriting optional?

As far as I'm concerned, defaulting to overwrite is good enough.

> Tools/Scripts/generate-w3c-testsuite-manifest:122
> +        os.remove(manifestFile)

Why do this? open(FILE, "w") truncates the file if it exists.

> Tools/Scripts/generate-w3c-testsuite-manifest:138
> +    for reftest in reftestList:

While this form is correct, too, the for should be in the else block and not in the same block with the if/else block.

> Tools/Scripts/generate-w3c-testsuite-manifest:143
> +main()

The main() call should be inside a check like this:

if __name__ == "__main__":
    main()

> Tools/Scripts/generate-w3c-testsuite-manifest:146
> +    

You should have at most one empty line at the end of the file

> Tools/Scripts/import-w3c-tests:41
> +importList = []

Maybe I'm nitpicking here, but choose one naming scheme and stick to it :) (either camelCase or underscore_notation)

> Tools/Scripts/import-w3c-tests:53
> +    importTests()

I'd rather have the function names contain whole words (Directory vs. Dir, Arguments vs. Args, Destination vs. Dest)

> Tools/Scripts/import-w3c-tests:56
> +

I recommend using optparse ( http://docs.python.org/2/library/optparse.html ) instead of manually parsing the arguments. It's a lot nicer, clearer and  solves both the parsing of the arguments and the displaying of the usage for the script.

> Tools/Scripts/import-w3c-tests:139
> +    

Ditto ^^ about blank lines between function declaration and function body.

> Tools/Scripts/import-w3c-tests:149
> +    global changeset

I'd rather have getChangeset() return a changeset and have this return value assigned to the global changeset value in the calling function than manipulating the global variable like this.

The same applies to the other uses of global variables (where feasible).

> Tools/Scripts/import-w3c-tests:157
> +        # Oh well, we tried

Shouldn't we also log something, maybe asking the user to install hg?

> Tools/Scripts/import-w3c-tests:162
> +    global test_status

Ditto L149

> Tools/Scripts/import-w3c-tests:179
> +        

Ditto about empty lines between for/if/else and their corresponding code blocks.

> Tools/Scripts/import-w3c-tests:213
> +               not(fullpath in skipList): 

I'd "rephrase" this if with affirmative conditions (e.g. if filename.startWith() or filename.endswith() or ...). This would also allow to drop the line breaks and have the whole condition on just one line.

> Tools/Scripts/import-w3c-tests:258
> +    tmpTestList = []

Wouldn't it be better (faster, clearer) to check for duplicates at the end of the function? Rather than using this temporary duplicate list?

> Tools/Scripts/import-w3c-tests:335
> +                newFilePath = newFilePath.replace("xht", "xhtml")

Why is this needed?

> Tools/Scripts/import-w3c-tests:431
> +def verbose(msg):

A matter of personal preference, but I think it would be nicer to have just one log function, that could receive a 'verbose' parameter and log or not the message, based on the presence of the -v command line argument.

[Dirk Pranke]

Comment on attachment 192111 [details]
one line fix for something silly

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

Here's some general high-level comments about writing Python code in a manner more resembling the rest of WebKit's python code :) :

> Tools/Scripts/generate-w3c-testsuite-manifest:16
> +from HTMLParser import HTMLParser

I don't think the HTMLParser library is particularly robust. We generally use BeautifulSoup for parsing HTML in webkitpy.

> Tools/Scripts/generate-w3c-testsuite-manifest:35
> +def main():

Generally speaking, we don't tend to use a lot of free (file-level) functions in webkit python code. The top-level executable scripts (in Tools/Scripts) tend to be very short wrappers around modules that live under Tools/Scripts/webkitpy. Even there, we usually hang functions off of classes and objects, unless the functions are pure (have no side effects).

>> Tools/Scripts/generate-w3c-testsuite-manifest:56
>> +        testPath = sys.argv[1]
> 
> As there's no advanced processing of the parameters going on, I'd rather drop the testPath global variable and make the scanTestDir() method accept sys.argv[1] as a parameter.

In webkitpy we invariably use the optparse module for option parsing and usage messages. Please look into that rather than rolling your own parsing.

>> Tools/Scripts/generate-w3c-testsuite-manifest:61
>> +    ignoreFiles = ["-ref", "-notref", ".css", ".js", ".png", ".jpg", ".jpeg", ".DS_Store", ".mf", ".log"]
> 
> If these are just used as parameters to other functions, I'd rather have them as script constants/variables.

script-level (global) constants are fine, but we don't like mutable globals; they're much harder to test and mock-out.

> Tools/Scripts/generate-w3c-testsuite-manifest:96
> +    f = open(testFilePath)

Also, as far as making things more webkitpy-like goes, we frown on direct access to os-level services like open(). Please look into the Host abstraction in webkitpy/common/host, and the FileSystem and Executive abstractions in webkitpy/common/system . We have all this stuff so that it is much easier to write unit tests for our code.

[Rebecca Hauck]

Created attachment 194275 [details]
new patch to incorporate feedback

Ok, this has been completely revised to incorporate all the feedback here - split into modules + classes, unit tests added.  There is no longer a manifest file and no longer a manual step. The vendor prefixing is now handled by loading the latest list from the Webkit source and looking for those properties in the tests. The parsing and conversion code has tests, but I probably still need to add tests for the actual import code that needs real directories of W3C tests to work. I wasn't sure how to do that and would like the reviewer's input. There is at least one test for importing a directory with no tests in it.

[Rebecca Hauck]

Created attachment 194309 [details]
updated patch - previous patch was missing a local uncommitted change

previous patch was missing a local change - otherwise, same the comment:

new patch to incorporate feedback 

Ok, this has been completely revised to incorporate all the feedback here - split into modules + classes, unit tests added.  There is no longer a manifest file and no longer a manual step. The vendor prefixing is now handled by loading the latest list from the Webkit source and looking for those properties in the tests. The parsing and conversion code has tests, but I probably still need to add tests for the actual import code that needs real directories of W3C tests to work. I wasn't sure how to do that and would like the reviewer's input. There is at least one test for importing a directory with no tests in it.

[Rebecca Hauck]

Comment on attachment 194309 [details]
updated patch - previous patch was missing a local uncommitted change

retracting, not yet ready for review

[Rebecca Hauck]

Created attachment 194324 [details]
Now ready for review

Last change before getting review feedback

[Dirk Pranke]

Comment on attachment 194324 [details]
Now ready for review

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

This is a *lot* closer to webkitpy style ... thank you! I've only done a cursory pass over this so far, so here's some minor stylistic nits. I wouldn't bother making the changes yet until I can do a substantive review, but it looks like you're on the right path.

> Tools/ChangeLog:5
> +        You must have checked log the W3C repository to your local drive.

You largely appear to repeat the same text down in test_importer.py, so I'd just refer to that instead of copying it here.

nit: "checked out", not "checked log" :)

> Tools/ChangeLog:39
> +        - All files are converted to work in WebKit:

I think (as I suspect you do also) we will all be happier down the road the closer we can get to the "conversion" process being a no-op. Perhaps we should file separate bugs to track each issue we find as they come up. Here's a few comments in the meantime ...

> Tools/ChangeLog:40
> +            1. .xht extensions are changed to .xhtml to make new-run-webkit-tests happy

we should probably just make new-run-webkit-tests recognize .xht .

> Tools/ChangeLog:46
> +               new-run-webkit-tests expects

do we have any guesses as to how many extra copies of tests this creates? It might be more reasonable to insert shim -expected.html files that do redirects or iframe the actual references. This would probably require some buy-in from webkit-dev, so copies are good for now.

> Tools/ChangeLog:64
> +        Reviewed by NOBODY (OOPS!).

lines 61-64 should be at the top of the changelog, right under your name.

> Tools/Scripts/webkitpy/w3c/test_converter.py:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

Presumably this should be (c) 2013 Adobe ...

> Tools/Scripts/webkitpy/w3c/test_converter.py:25
> +from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup as Parser

why "as Parser" ?

> Tools/Scripts/webkitpy/w3c/test_importer.py:26
> +############################################################################################################

this should be a docstring rather than a comment. If you do that, then 'pydoc webkitpy.w3c.test_importer' will print this automatically :).

[Dirk Pranke]

Comment on attachment 194324 [details]
Now ready for review

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

Okay, as per our discussion over IM/IRC earlier today, I think it makes sense to land this and iterate as soon as we can, even if the code isn't ideal. The scripts shouldn't break anything else or affect anyone's checkout in particular.

Please address as many of the stylistic nits as you can (especially the copyright and changelog info). Don't worry about cleaning up the tests too much. Re-post another patch just so we can be sure it'll still apply against the tree and we should be able to land something soon.

Thanks for bearing with me during the long delay!

> Tools/Scripts/webkitpy/w3c/test_converter.py:125
> +        @return True if changes were made

We don't usually use javadoc-style @param/@return notation for parameters. We would probably do something more like:

def convert_testharness_paths(self, doc, new_path):
   """Update links to testharness.js in the BeautifulSoup |doc| to point to the copy in |new_path|. Returns whether the document was modified."""

I.e., you can get a lot terser. A "proper" pythonic docstring has a one-line summary, then a blank line, then the rest of the string, but we don't tend to stick to this that carefully and favor shorter docstrings (or none at all) where they're not really needed.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

update copyright here as well.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:77
> +"""

There's a lot of boilerplate text in these tests, and it's hard to tell which parts of the string are relevant and change from test to test. We should try to wrap the test html in a generator function or something.

> Tools/Scripts/webkitpy/w3c/test_importer.py:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

update the copyright.

> Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

update.

> Tools/Scripts/webkitpy/w3c/test_parser.py:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

update.

> Tools/Scripts/webkitpy/w3c/test_parser_unittest.py:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

:).

> Tools/Scripts/webkitpy/w3c/test_parser_unittest.py:98
> +"""

same comments about repeat blocks of text.

[Rebecca Hauck]

Created attachment 200295 [details]
Incorporated Dirk's feedback

Ok, I think I covered everyting - fixed the pydoc comments, removed unnecessary text from tests, cleaned up the changelog.

[Dirk Pranke]

Comment on attachment 200295 [details]
Incorporated Dirk's feedback

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

Looks pretty good, so I'm R+'ing it with a few more nits. Feel free to fix those and land w/o another review. Once we do that, if you want to use this script to import specific subdirs I think that's great but we should probably do so on an ad-hoc basis into someplace *other* than where we might want the long-term location of the whole w3 repo to be.

In other words, if we end up wanting to put the w3c's tests in LayoutTests/w3c/... , don't import submitted/adobe/regions (or whatever) into LayoutTests/w3c/submitted/adobe/regions directly. You should probably have me review the first import or two just to see how it goes.

Thanks so much for working on this!

> Tools/Scripts/import-w3c-tests:3
> +# Copyright (C) 2010 Apple Inc. All rights reserved.

copyright :)

> Tools/Scripts/webkitpy/w3c/test_converter.py:31
> +from webkitpy.thirdparty.BeautifulSoup import Tag

nit: list the standard modules first, then the stuff from webkitpy.thirdparty. Also, you can (and should) do "import BeautifulSoup, Tag"

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

nit: don't put a blank line here.

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

nit: don't put blank lines between your method declarations and the body of the methods.

[Rebecca Hauck]

Created attachment 200314 [details]
Fixed that last few minor nits

[Rebecca Hauck]

Created attachment 200319 [details]
Added Dirk Pranke as reviewer

[WebKit Commit Bot]

Comment on attachment 200319 [details]
Added Dirk Pranke as reviewer

Rejecting attachment 200319 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 200319, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
angeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/Scripts/import-w3c-tests
patch: **** Only garbage was found in the patch input.
fatal: pathspec 'Tools/Scripts/webkitpy/w3c/__init__.py' did not match any files
Failed to git add Tools/Scripts/webkitpy/w3c/__init__.py. at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 448.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Dirk Pranke']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/357160

[Rebecca Hauck]

Created attachment 200462 [details]
Attempting to fix the __init__.py issue

[Dirk Pranke]

Note that you should be able to verify that the patch is okay by running 'webkit-patch apply-attachment' on the attachment. (Of course, not on the same branch that you just uploaded the patch from).

[Rebecca Hauck]

Comment on attachment 200462 [details]
Attempting to fix the __init__.py issue

EWS all green - hopefully this one works.

[Dirk Pranke]

Comment on attachment 200462 [details]
Attempting to fix the __init__.py issue

let's find out.

[WebKit Commit Bot]

Comment on attachment 200462 [details]
Attempting to fix the __init__.py issue

Clearing flags on attachment: 200462

Committed r149547: <http://trac.webkit.org/changeset/149547>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

yay!

[Zan Dobersek]

Comment on attachment 200462 [details]
Attempting to fix the __init__.py issue

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

The webkitpy.w3c.test_importer_unittest.TestImporterTest.test_ImportDirWithNoTests unit test is failing after the patch landed in r149547.
http://trac.webkit.org/changeset/149547

A couple of problems are noted in the inline comments.

> Tools/Scripts/webkitpy/w3c/test_converter.py:190
> +                print 'converting ' + unprefixed_prop + ' -> ' + prefixed_prop

Printing the progress messages through the print statement causes unnecessary output when running the tests. Using Python loggers (i.e. _log.info and similar) instead would be much better, capturing the output of the tests. This isn't critical, but it is rather annoying.

> Tools/Scripts/webkitpy/w3c/test_importer.py:345
> +                    if options.no_overwrite is True and os.path.exists(new_filepath):

The unit test fails here due to no global options object available. Referencing self.options instead fixes this.
The alternative would be to define the global options object, as done in test_parser_unittest.py.

> Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:47
> +        importer = TestImporter(dir_with_no_tests, None)

If switched to using self.options to check for the no_overwrite option in TestImporter.import_tests, you can pass a MockOptions object as the options parameter. For instance, MockOptions(no_overwrite=False) should do the trick.

[Zan Dobersek]

I've just now noted that when the unit tests do pass (after addressing the options object issues), the LayoutTests/csswg/s directory is populated with most of the files under Source/WebCore/css.

I don't think it's a good idea to have the unit test operate on the source checkout instead of a mock filesystem representation (which is indeed noted as a TODO item in the test_importer_unittest.py file).

I'd recommend disabling this specific unit test until the testing moves onto a mocked filesystem.

[Dirk Pranke]

I didn't actually run the tests to notice the problems, so this is a failure on my part. You're absolutely correct that the tests should not log to stdout (at least not w/o having called OutputCapture().capture_output() first), and definitely not be writing to the real filesystem. 

We should disable the tests until at least the latter is fixed. I will try to get to this today or tomorrow.

[Ryosuke Niwa]

I briefly looked into disabling these tests, but there are so many of them and test_converter_unittest.py is spitting out a bunch of stuff as well. I’m going to roll out the patch for now since nobody has been dependent on this tool yet.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 115681

[Dirk Pranke]

Created attachment 201279 [details]
cleaned-up patch for relanding

[Dirk Pranke]

Created attachment 201281 [details]
delta from patch that landed initially, for ease of review

[Dirk Pranke]

benjaminp, rniwa, can one of you take a look at the revised patch? (Just looking at the delta attachment should be fine, since that shows the changes from the version I R+'ed/landed initially).

As I note in the ChangeLog, this code still isn't ideal as it's not yet using any of the webkitpy system abstractions (host, filesystem, etc.). If it's okay, I'd like to land the patch as-is and then incrementally clean up the code to do so.

[Ryosuke Niwa]

Comment on attachment 201279 [details]
cleaned-up patch for relanding

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

> Tools/Scripts/webkitpy/w3c/test_converter.py:43
> +class TestConverter(object):

This class name doesn’t tell us what we’re converting from and into but maybe it’s okay since it’s inside webkitpy/w3c.

> Tools/Scripts/webkitpy/w3c/test_converter.py:55
> +        f = open(os.path.join(os.path.sep, WEBKIT_ROOT, SOURCE_SUBDIR, WEBCORE_SUBDIR,
> +                                           CSS_SUBDIR, CSS_PROPERTY_NAMES_IN))

We need to share more code with the rest of webkitpy.

> Tools/Scripts/webkitpy/w3c/test_converter.py:65
> +                    continue

Nit: wrong indentation. continue must be exactly 4 spaces to the right of line.split.

> Tools/Scripts/webkitpy/w3c/test_converter.py:76
> +        f = open(filename)
> +        contents = f.read()
> +        f.close()

Why are we not using FileSystem instead?

> Tools/Scripts/webkitpy/w3c/test_converter.py:101
> +        if len(converted[0]) > 0 or jstest is True:
> +            return converted
> +        else:
> +            return None

No else clause after return. Also, I would have used tertiary here as in:
return converted if converted[0] or jstest else None

Also, we shouldn’t be comparing len() with 0 since len is never negative.
Furthermore, len is completely unnecessary.

> Tools/Scripts/webkitpy/w3c/test_converter.py:112
> +        if len(testharness_tags) != 0:

Nit: we shouldn’t be comparing with 0. Also, we prefer early exit.
So do:
if not testharness_tags:
    return False

> Tools/Scripts/webkitpy/w3c/test_converter.py:116
> +            resources_path = os.path.join(os.path.sep, WEBKIT_ROOT, LAYOUT_TESTS_DIRECTORY, RESOURCES_DIRECTORY)
> +            resources_relpath = os.path.relpath(resources_path, new_path)

It seems like we need a helper function to make a path relative to LayoutTests.

> Tools/Scripts/webkitpy/w3c/test_converter.py:148
> +            # Get the text whether in a style tag or style attribute

It’s better to define a helper method to do this (you can even do nested functions) rather than adding a comment like this.

> Tools/Scripts/webkitpy/w3c/test_converter.py:151
> +                if len(tag.contents) == 0:

Nit: again, we we don’t need len.

> Tools/Scripts/webkitpy/w3c/test_converter.py:158
> +            # Scrub it for props requiring prefixes

This comment simply repeats what the code says. Please remove it.
Better yet, clarify why we’re doing it.

> Tools/Scripts/webkitpy/w3c/test_converter.py:161
> +            # Rewrite tag only if changes were made

Ditto about repeating the code.

> Tools/Scripts/webkitpy/w3c/test_converter.py:162
> +            if len(scrubbed_style[0]) > 0:

Nit: len(~) > 0 -> ~.

> Tools/Scripts/webkitpy/w3c/test_converter.py:165
> +                # Build a new tag with the modified text

Ditto.

> Tools/Scripts/webkitpy/w3c/test_converter.py:169
> +                # Update the doc with the new tag

Ditto. All these comments are adding so much noise to the code.

> Tools/Scripts/webkitpy/w3c/test_converter.py:179
> +        # Spin through the whole list of prefixed properties

Again, this comment adds nothing beyond what the line below tells me.

> Tools/Scripts/webkitpy/w3c/test_converter.py:182
> +            # Pull the prefix off

Ditto.

> Tools/Scripts/webkitpy/w3c/test_converter.py:194
> +        # TODO: Handle the JS versions of these properties, too.

Nit: We use FIXME, not TODO.

> Tools/Scripts/webkitpy/w3c/test_converter.py:201
> +        # Replace the previous tag with the new one

Again, this comment simply repeats the code and the method name.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:31
> +import os
> +from random import randint
> +import re

two imports (without from) should probably appear next to each other.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:46
> +        # Get the full list of prefixed properties

Why are we adding this comment instead of renaming prop_list to all_prefixed_properties?

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:50
> +        # Verify we got properties back

This comment repeats the code beneath it again.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:53
> +        # Verify they're all prefixed

Ditto.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:80
> +        # Try to convert the html

Ditto.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:88
> +        # Verify nothing was converted

Ditto.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:101
> +        # Create path to a fake test directory

Ditto.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:105
> +                                              test_converter.LAYOUT_TESTS_DIRECTORY,
> +                                              test_converter.CSS_SUBDIR,
> +                                              'harnessonly')

Wrong indentation. All subsequent lines should appear exactly 4 spaces to the right of test_path =.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:116
> +        # Convert the html
> +        converted = converter.convert_for_webkit(test_path, contents=test_html, filename='somefile.html')
> +
> +        # Verify a conversion happened
> +        self.assertNotEqual(converted, None, 'test was not converted')
> +
> +        # Verify that both the harness paths were converted correctly
> +        self.verifyTestHarnessPaths(converted[1], test_path, 1, 1)
> +
> +        # Verify no properties were converted

Ditto about comments.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:140
> +        # Create path to a fake test directory

Ditto.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:144
> +        test_path = os.path.join(os.path.sep, test_converter.WEBKIT_ROOT,
> +                                              test_converter.LAYOUT_TESTS_DIRECTORY,
> +                                              test_converter.CSS_SUBDIR,
> +                                              'harnessandprops')

Ditto about indentation.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:160
> +        # Generate & insert the test properties into the test_html
> +        test_content = self.generateTestContent(converter.prefixed_properties, 2, test_html)
> +
> +        # Convert the html
> +        oc = OutputCapture()
> +        oc.capture_output()
> +        try:
> +            converted = converter.convert_for_webkit(test_path, contents=test_content[1], filename='somefile.html')
> +        finally:
> +            oc.restore_output()
> +
> +        # Verify a conversion happened
> +        self.assertNotEqual(converted, None, 'test was not converted')
> +
> +        # Verify that both the harness paths and properties were all converted correctly

Ditto about comments. And dittos about indentation and comments for all subsequent tests.

> Tools/Scripts/webkitpy/w3c/test_importer.py:126
> +    parser.add_option('-n', '--no-overwrite',
> +                      #action='store_true',
> +                      default=False,
> +                      help='Flag to prevent duplicate test files from overwriting existing tests. By default, they will be overwritten')

Wrong indentation. Subsequent lines should be exactly 4 spaces to the right of parser.add_option.
Also, we don’t normally add commented out line.

> Tools/Scripts/webkitpy/w3c/test_importer.py:130
> +                      action='store_true',
> +                      default=False,
> +                      help='Import all tests including reftests, JS tests, and manual/pixel tests. By default, only reftests and JS tests are imported')

Ditto about indentations.

> Tools/Scripts/webkitpy/w3c/test_importer.py:142
> +    # Make sure the w3c test directory exists

Again, all these comments that simply repeat the code are adding so much noise.

> Tools/Scripts/webkitpy/w3c/test_importer.py:149
> +    if import_dir.find(APPROVED) == -1 and \
> +       import_dir.find(SUBMITTED) == -1:

It seems like these two lines can fit in one line. If not, then the number of spaces for the indentation is wrong.

> Tools/Scripts/webkitpy/w3c/test_importer.py:151
> +        # If not pointed directly to the approved directory or to any submitted
> +        # directory, check for a submitted subdirectory and go with that

This is a what comment again. I’d like to know why we do that since I can already tell what it does from the code.

> Tools/Scripts/webkitpy/w3c/test_importer.py:188
> +        except:
> +            # Oh well, we tried
> +            self.changeset = 'Not Available'

Can we output the message attached to the exception?
Also please remove the comment.

> Tools/Scripts/webkitpy/w3c/test_importer.py:213
> +            for filename in files:

It seems like the content of this for loop can be extracted as a helper function.

> Tools/Scripts/webkitpy/w3c/test_importer.py:219
> +                if 'html' in str(mimetype[0]) or 'xml' in str(mimetype[0]):

Or inside this if.

> Tools/Scripts/webkitpy/w3c/test_importer.py:249
> +                        # If there are ref support files, the destination is should now be relative to the
> +                        # test file and new location of the ref file

Again, this is what comment. We should explain whit should be relative to the test file instead.

> Tools/Scripts/webkitpy/w3c/test_importer.py:257
> +                                # Build the correct source path
> +                                source_file = os.path.join(os.path.dirname(test_info['reference']), support_file)
> +                                source_file = os.path.normpath(source_file)
> +                                # Keep the dest as it was
> +                                to_copy = {'src': source_file, 'dest': support_file}
> +                                # Only add it once

All these comments seem unnecessary.

> Tools/Scripts/webkitpy/w3c/test_importer.py:271
> +                # Ignore dotfiles and random perl scripts that are in some dirs
> +                elif not(filename.startswith('.')) and not(filename.endswith('.pl')):

Why are we not doing this first? Presumably checking the filename is faster than guessing the mime type, right?

> Tools/Scripts/webkitpy/w3c/test_importer.py:274
> +            if total_tests == 0:

Nit: Don’t compare  with 0.
Do “if not total_tests” instead.

> Tools/Scripts/webkitpy/w3c/test_importer.py:280
> +                if len(copy_list) > 0:

Nit: len(~) > 0 -> ~

> Tools/Scripts/webkitpy/w3c/test_importer.py:283
> +                                            'reftests': reftests, 'jstests': jstests, 'total_tests': total_tests})

Nit: wrong indentation.

> Tools/Scripts/webkitpy/w3c/test_importer.py:289
> +        if len(self.import_list) > 0:

Nit: len(~) > 0 -> ~

> Tools/Scripts/webkitpy/w3c/test_importer.py:305
> +            if len(dir_to_copy['copy_list']) > 0:

Ditto. Again, we can extract a helper function here.

> Tools/Scripts/webkitpy/w3c/test_importer.py:315
> +                # Append the new subpath to the destination_directory
> +                new_path = os.path.join(self.destination_directory, new_subpath)
> +
> +                # Create the destination subdirectories if not there

More useless comments.

> Tools/Scripts/webkitpy/w3c/test_importer.py:327
> +                    # Shouldn't be any directories on the list, but check to be safe
> +                    if os.path.isdir(orig_filepath):
> +                        continue

Why don’t we assert instead?

> Tools/Scripts/webkitpy/w3c/test_importer.py:330
> +                    # Don't choke if the file can't be found
> +                    if not(os.path.exists(orig_filepath)):

Why do we ever encounter this case? Also, this if can be merged with the previous if.

> Tools/Scripts/webkitpy/w3c/test_importer.py:334
> +                    # Append the correct destination filename

Nit: Repeats the code.

> Tools/Scripts/webkitpy/w3c/test_importer.py:340
> +                    # Make the directories needed

Ditto.

> Tools/Scripts/webkitpy/w3c/test_importer.py:344
> +                    # Don't overwrite existing tests if specified

Ditto.

> Tools/Scripts/webkitpy/w3c/test_importer.py:348
> +                        # TODO: Maybe doing a file diff is in order here for existing files?

Use FIXME.

> Tools/Scripts/webkitpy/w3c/test_importer.py:355
> +                    # TODO: Eventually, so should js when support is added for this type of conversion

Ditto.

> Tools/Scripts/webkitpy/w3c/test_importer.py:359
> +                    if 'html' in str(mimetype[0]) or \
> +                       'xml' in str(mimetype[0])  or \
> +                       'css' in str(mimetype[0]):

It seems like we can fit in one line. If not, then we have a wrong indentation here as well.

> Tools/Scripts/webkitpy/w3c/test_importer.py:368
> +                            # Write out the converted test

Nit: Repeats the code.

> Tools/Scripts/webkitpy/w3c/test_importer.py:378
> +                # Take care of anything that may have been removed since the last import

“Take care” is vague. What/how are we taking care of?

> Tools/Scripts/webkitpy/w3c/test_importer.py:379
> +                self.remove_deleted_files(new_path, copied_files)

What does remove_delete_files mean?

> Tools/Scripts/webkitpy/w3c/test_importer.py:381
> +                # Drop some info about what just happened here

Nit: Repeats the code.

> Tools/Scripts/webkitpy/w3c/test_importer.py:395
> +        # The test status is in the path, so grab it
> +        self.get_test_status()

I don’t understand what these two lines of code are doing at all.
Why is a function named self.get_test_status() not returning anything, and we’re calling it here for what reason?

> Tools/Scripts/webkitpy/w3c/test_importer.py:409
> +    def get_test_status(self):
> +        """ Sets the test status to either 'approved' or 'submitted' """

I don’t think we should call this function get_test_status if it merely updates the internal state.
Why don’t we simply use @memoized and make this function return the status and get rid of self.test_status?

> Tools/Scripts/webkitpy/w3c/test_importer.py:441
> +        # Loop through and remove them all

Nit: Repeats the code.

> Tools/Scripts/webkitpy/w3c/test_importer.py:443
> +            print 'Deleting file removed from the W3C repo:' + deleted_file

Nit: Use “,” instead of “+” here and elsewhere.

> Tools/Scripts/webkitpy/w3c/test_importer.py:467
> +        if len(prop_list) == 0:

Nit: len(~) == 0 -> not ~.

> Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:50
> +        dir_with_no_tests = os.path.join(os.path.sep, test_converter.WEBKIT_ROOT,
> +                                                      test_converter.SOURCE_SUBDIR,
> +                                                      test_converter.WEBCORE_SUBDIR,
> +                                                      test_converter.CSS_SUBDIR)

Nit: wrong indentation.

> Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:62
> +    # TODO: Need more tests, but what to use for valid source directory? Make it on the fly

Nit: use FIXME.

[Build Bot]

Comment on attachment 201279 [details]
cleaned-up patch for relanding

Attachment 201279 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/395046

New failing tests:
svg/batik/filters/feTile.svg

[Build Bot]

Created attachment 201298 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Ryosuke Niwa]

Comment on attachment 201279 [details]
cleaned-up patch for relanding

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

> Tools/Scripts/webkitpy/w3c/test_parser.py:50
> +            f = open(file_to_parse)
> +            html = f.read()
> +            f.close()

This duplicates the code in TestConverter.

> Tools/Scripts/webkitpy/w3c/test_parser.py:64
> +        if test_contents is not None:

Nit:
if test_contents:

> Tools/Scripts/webkitpy/w3c/test_parser.py:67
> +        if ref_contents is not None:

Ditto.

> Tools/Scripts/webkitpy/w3c/test_parser.py:72
> +        if len(matches) != 0:

Nit: Don’t compare with 0.

> Tools/Scripts/webkitpy/w3c/test_parser.py:103
> +        elif self.options['all'] is True and \
> +             not('-ref' in self.filename) and \
> +             not('reference' in self.filename):

It seems like all of this fit in one line. If not, then we have a wrong indentation.

> Tools/Scripts/webkitpy/w3c/test_parser.py:107
> +        # Courtesy check and warning if there are any mismatches
> +        mismatches = self.get_reftests(False)

We do! r- because of this.

> Tools/Scripts/webkitpy/w3c/test_parser.py:108
> +        if len(mismatches) != 0:

Nit: Don’t compare with 0.

> Tools/Scripts/webkitpy/w3c/test_parser.py:119
> +        if find_match:
> +            return self.test_doc.findAll(rel='match')
> +        else:
> +            return self.test_doc.findAll(rel='mismatch')

Why not
return self.test_doc.findAll(rel=‘match’ if find_match else 'mismatch’)

> Tools/Scripts/webkitpy/w3c/test_parser.py:127
> +        if self.test_doc.find(src=re.compile('[\'\"/]?/resources/testharness')) is None:
> +            return False
> +        else:
> +            return True

return not self.test_doc.find(src=re.compile('[\'\"/]?/resources/testharness’)).

> Tools/Scripts/webkitpy/w3c/test_parser.py:141
> +        # Look for tags with src or href attributes
> +        src_attrs = doc.findAll(src=re.compile('.*'))
> +        href_attrs = doc.findAll(href=re.compile('.*'))
> +
> +        # Look for urls

These two comments repeat the code.

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

Fit in one line.

> Tools/Scripts/webkitpy/w3c/test_parser_unittest.py:53
> +        # Verify the correct test info was found

Nit: Repeat the code, and ditto elsewhere in the tests.

[Dirk Pranke]

Created attachment 201312 [details]
update w/ review feedback

[Dirk Pranke]

Lots of cleanup; the code should at least resemble webkitpy style now, although there's a few more minor things still to convert over.

Please take another look?

[Dirk Pranke]

rniwa, benjaminp, can one of you take another look at this?

[Benjamin Poulain]

Comment on attachment 201312 [details]
update w/ review feedback

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

Some minor comments:

> Tools/Scripts/webkitpy/w3c/test_converter.py:42
> +        self.host = Host()
> +        self.filesystem = self.host.filesystem
> +        self._webkit_root = __file__.split(self.filesystem.sep + 'Tools')[0]
> +        self.prefixed_properties = self.load_prefixed_prop_list()

Why only one name prefixed with _?

> Tools/Scripts/webkitpy/w3c/test_converter.py:48
> +        """ Reads the current list of CSS properties requiring the -webkit- prefix from Source/WebCore/CSS/CSSPropertyNames.in and stores them in an instance variable """

I would not put the full path, just CSSPropertyNames.in.

> Tools/Scripts/webkitpy/w3c/test_converter.py:52
> +        # Open CSSPropertyNames.in to get the current list of things requiring prefixes

Missing period.

> Tools/Scripts/webkitpy/w3c/test_converter.py:55
> +            # Look for anything with the -webkit- prefix

Missing period.

> Tools/Scripts/webkitpy/w3c/test_converter.py:56
> +            match = re.search('^-webkit-[\w|-]*', line)

Not sure why you use re.search() and not re.match() here.

> Tools/Scripts/webkitpy/w3c/test_converter.py:59
> +                # Ignore the ones where both the prefixed and non-prefixed property
> +                # are supported - denoted by -webkit-some-property = some-property

Missing period.

> Tools/Scripts/webkitpy/w3c/test_converter.py:60
> +                if len(line.split('=')) == 2 and line.split('=')[1].strip() in line.split('=')[0].strip():

put "line.split('=')" in a temporary variable and you can greatly simplify the expression.

> Tools/Scripts/webkitpy/w3c/test_converter.py:71
> +        if contents is None and filename is None:
> +            return None

if not contents and not filename: ?

> Tools/Scripts/webkitpy/w3c/test_converter.py:161
> +            pattern = '([\s{]|^)' + unprefixed_prop + '(\s+:|:)'

Maybe '([\s{]|^)(%s)(\s+:|:)' % foobar to make that regexp easier to read?

[Dirk Pranke]

Comment on attachment 201312 [details]
update w/ review feedback

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

>> Tools/Scripts/webkitpy/w3c/test_converter.py:42
>> +        self.prefixed_properties = self.load_prefixed_prop_list()
> 
> Why only one name prefixed with _?

It's conceivable that host, filesystem, and prefixed_properties are public, but I know (having added) that _webkit_root isn't. That said, I don't have a great answer. I will go through and make everything that doesn't need to be pubilc private.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:48
>> +        """ Reads the current list of CSS properties requiring the -webkit- prefix from Source/WebCore/CSS/CSSPropertyNames.in and stores them in an instance variable """
> 
> I would not put the full path, just CSSPropertyNames.in.

Sure, will fix.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:52
>> +        # Open CSSPropertyNames.in to get the current list of things requiring prefixes
> 
> Missing period.

Will fix.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:55
>> +            # Look for anything with the -webkit- prefix
> 
> Missing period.

Will fix.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:56
>> +            match = re.search('^-webkit-[\w|-]*', line)
> 
> Not sure why you use re.search() and not re.match() here.

there's probably no good reason. I will change it to re.match().

>> Tools/Scripts/webkitpy/w3c/test_converter.py:59
>> +                # are supported - denoted by -webkit-some-property = some-property
> 
> Missing period.

will fix.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:60
>> +                if len(line.split('=')) == 2 and line.split('=')[1].strip() in line.split('=')[0].strip():
> 
> put "line.split('=')" in a temporary variable and you can greatly simplify the expression.

good suggestion.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:71
>> +            return None
> 
> if not contents and not filename: ?

contents='' would actually be a legal value, but looking at it again, it's probably safe to exit early even if it is. It looks like contents= is only used for unit-testing, and we should probably refactor this routine accordingly so that we're not using optional parameters and branches here.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:161
>> +            pattern = '([\s{]|^)' + unprefixed_prop + '(\s+:|:)'
> 
> Maybe '([\s{]|^)(%s)(\s+:|:)' % foobar to make that regexp easier to read?

I'm not sure that that's any easier to read to me, and that would introduce a new match group (though I think you don't need the () around the %s).

[Ryosuke Niwa]

Comment on attachment 201312 [details]
update w/ review feedback

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

> Tools/Scripts/webkitpy/w3c/test_converter.py:41
> +        self._webkit_root = __file__.split(self.filesystem.sep + 'Tools')[0]

Why aren't we using scm.checkout_root? We should at least have a FIXME.

> Tools/Scripts/webkitpy/w3c/test_converter.py:48
> +    def load_prefixed_prop_list(self):
> +        """ Reads the current list of CSS properties requiring the -webkit- prefix from Source/WebCore/CSS/CSSPropertyNames.in and stores them in an instance variable """

Why do we need this comment?
Why can't we rename the method to load_webkit_prefixed_css_property_list?

Also, we shouldn't abbreviate property as prop.

> Tools/Scripts/webkitpy/w3c/test_converter.py:53
> +        contents = self.filesystem.read_text_file(self.path_from_webkit_root('Source', 'WebCore', 'css', 'CssPropertyNames.in'))

Ditto.

> Tools/Scripts/webkitpy/w3c/test_converter.py:71
> +        if contents is None and filename is None:
> +            return None

Explicitly comparing to None seems bogus. We should simply do "if not contents and not filename" instead.

> Tools/Scripts/webkitpy/w3c/test_converter.py:73
> +        if contents is None:

Ditto.

> Tools/Scripts/webkitpy/w3c/test_converter.py:76
> +        # Handle plain old CSS files

If comment doesn't add any value.

> Tools/Scripts/webkitpy/w3c/test_converter.py:118
> +        converted_props = []

We should spell out properties.

> Tools/Scripts/webkitpy/w3c/test_converter.py:136
> +            scrubbed_style = self.scrub_unprefixed_props(style_text)

I would much prefer calling these as: add_webkit_prefix_to_unprefix_properties and prefixed_style_text or modified_style_text.

> Tools/Scripts/webkitpy/w3c/test_converter.py:150
> +    def scrub_unprefixed_props(self, text):
> +        """ Searches |text| for instances of properties requiring the -webkit- prefix and adds the prefix. Returns the list of converted properties and the modified string """

This method-level comment is redundant once we rename the method to add_webkit_prefix_to_unprefix_properties.

> Tools/Scripts/webkitpy/w3c/test_converter.py:163
> +                print 'converting ' + unprefixed_prop + ' -> ' + prefixed_prop

Do: "converting %s -> %s" % (unprefixed_prop, prefixed_prop)

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:45
> +    def testLoadPrefixedPropList(self):

Why is this camelCase'd? And ditto for other tests.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:293
> +        self.assertEquals(len(converted.findAll(src=orig_path_pattern)), 0,
> +                          'testharness src path was not converted')
> +        self.assertEquals(len(converted.findAll(href=orig_path_pattern)), 0,
> +                          'testharness href path was not converted')

Wrong indentation. Indent by 4 spaces.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:295
> +        # Get the new relpath from the tester dir

This comment should be removed.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:298
> +        # Verify it's in all the right places

This comment doesn't add any value because "right places" are not obvious.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:303
> +        self.assertEquals(len(converted.findAll(src=relpath_pattern)), num_src_paths,
> +                          'testharness src relative path not correct')
> +        self.assertEquals(len(converted.findAll(href=relpath_pattern)), num_href_paths,
> +                          'testharness href relative path not correct')

Ditto.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:306
> +    def verifyPrefixedProperties(self, converted, test_properties):
> +        """ Verifies a list of test_properties were converted correctly """

Again, camelCase shouldn't be used and we should rename the test to be more descriptive and remove the comment.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:308
> +        # Verify that the number of test properties equals the number that were converted

This comment repeats the code.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:311
> +        # Verify they're all in the converted document

Ditto.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:320
> +        # Grab a random bunch of 20 unique properties requiring prefixes to test with

We shouldn't be randomly picking properties to test. r- because of this.

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:323
> +            idx = random.randint(0, len(full_prop_list) - 1)

What does idx mean?

> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:324
> +            if not(full_prop_list[idx] in test_properties):

Why not "full_prop_list[idx] not in test_properties"

> Tools/Scripts/webkitpy/w3c/test_importer.py:130
> +    parser.add_option('-n', '--no-overwrite',
> +                      dest='overwrite',
> +                      action='store_false',
> +                      default=True,
> +                      help='Flag to prevent duplicate test files from overwriting existing tests. By default, they will be overwritten')
> +    parser.add_option('-a', '--all',
> +                      action='store_true',
> +                      default=False,
> +                      help='Import all tests including reftests, JS tests, and manual/pixel tests. By default, only reftests and JS tests are imported')
> +

Wrong indentations.

> Tools/Scripts/webkitpy/w3c/test_importer.py:178
> +    def get_changeset(self):

This should be renamed to something like load_tip_changeset.
Per style guideline, get_ prefix should be used for functions with an out argument.

> Tools/Scripts/webkitpy/w3c/test_importer.py:188
> +    def scan_source_directory(self, directory):
> +        """ Walks the |directory| looking for HTML files that are importable tests. """

Why can't we just rename this function find_importable_tests and get rid of the comment?

> Tools/Scripts/webkitpy/w3c/test_importer.py:213
> +                if filename.startswith('.') or filename.endswith('.pl'):
> +                    continue  # For some reason the w3c repo contains random perl scripts we don't care about.

It seems like the whole block inside this for should be a separate function.

> Tools/Scripts/webkitpy/w3c/test_importer.py:271
> +                    self.import_list.append({'dirname': root, 'copy_list': copy_list,
> +                                             'reftests': reftests, 'jstests': jstests, 'total_tests': total_tests})

Wrong indentation.

> Tools/Scripts/webkitpy/w3c/test_importer.py:274
> +    def import_tests(self):
> +        """ Copies and converts the full list of importable tests into Webkit and logs what was imported in each directory """

I don't think this comment is useful because "importing tests" in this context implies all of that.

> Tools/Scripts/webkitpy/w3c/test_importer.py:332
> +                    # TODO: Maybe doing a file diff is in order here for existing files?
> +                    #       In other words, there's no sense in overwriting identical files, but
> +                    #       there's no harm in copying the identical thing.

s/TODO/FIXME/. Also comments shouldn't be indented inside like this.

> Tools/Scripts/webkitpy/w3c/test_importer.py:334
> +                    print 'Importing: ' + orig_filepath
> +                    print '       As: ' + new_filepath

":', " instead of ": ' +"

> Tools/Scripts/webkitpy/w3c/test_importer.py:337
> +                # TODO: Eventually, so should js when support is added for this type of conversion

Ditto.

> Tools/Scripts/webkitpy/w3c/test_importer.py:344
> +                    if not converted_file:
> +                        shutil.copyfile(orig_filepath, new_filepath)  # The file was unmodified.

This comment repeats the code.

> Tools/Scripts/webkitpy/w3c/test_importer.py:381
> +        """ Sets the test status to either 'approved' or 'submitted' """

I don't think this comment is useful.

> Tools/Scripts/webkitpy/w3c/test_importer.py:385
> +        if self.source_directory.find('approved') != -1:

What happens if someone added a directory named preapproved? You probably need to filesystem.split and then find 'approved' token inside the list.

> Tools/Scripts/webkitpy/w3c/test_importer.py:387
> +        elif self.source_directory.find('submitted') != -1:

Same problem.

> Tools/Scripts/webkitpy/w3c/test_importer.py:398
> +        if not(os.path.exists(import_log_file)):

No parenthesis for not.

> Tools/Scripts/webkitpy/w3c/test_importer.py:405
> +            list_idx = contents.index('List of files:\n') + 1

We shouldn't abbreviate index as idx.

> Tools/Scripts/webkitpy/w3c/test_importer.py:407
> +            previous_file_list = map(lambda s: s.strip(), previous_file_list)

Please don't use one letter variable like s. file?

> Tools/Scripts/webkitpy/w3c/test_importer.py:433
> +        import_log.write('The tests in this directory were imported from the W3C repository.\n')
> +        import_log.write('Do NOT modify these tests directly in Webkit. Instead, push changes to the W3C CSS repo:\n\n')
> +        import_log.write('http://hg.csswg.org/test\n\n')
> +        import_log.write('Then run the Tools/Scripts/import-w3c-tests in Webkit to reimport\n\n')
> +        import_log.write('Do NOT modify or remove this file\n\n')
> +        import_log.write('------------------------------------------------------------------------\n')
> +        import_log.write('Last Import: ' + now.strftime('%Y-%m-%d %H:%M') + '\n')
> +        import_log.write('W3C Mercurial changeset: ' + self.changeset + '\n')
> +        import_log.write('Test status at time of import: ' + self.test_status + '\n')
> +        import_log.write('------------------------------------------------------------------------\n')
> +        import_log.write('Properties requiring vendor prefixes:\n')

Why are we not using triple-quotes here?

> Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:41
> +    def test_ImportDirWithNoTests(self):

No camelCase.

> Tools/Scripts/webkitpy/w3c/test_parser.py:110
> +    def get_reftests(self, rel):
> +        """ Searches for all reference links in the file, by looking for a rel of either "match" or "mismatch"."""

The method should be renamed to find_all_link_elements_with_rel, and the comment should be removed
because rel can be anything, and the function is generic enough that it can return any element with rel attribute.
I would much prefer for this function to return href of a link element instead though.
Nevertheless, the comment is inaccurate.

> Tools/Scripts/webkitpy/w3c/test_parser.py:114
> +        """ Searches the file for usage of W3C-style testharness paths """

This comment should be removed.

> Tools/Scripts/webkitpy/w3c/test_parser.py:125
> +        src_attrs = doc.findAll(src=re.compile('.*'))
> +        href_attrs = doc.findAll(href=re.compile('.*'))

These variables should be renamed to elements_with_src_attribute and elements_with_href_attribute

> Tools/Scripts/webkitpy/w3c/test_parser_unittest.py:42
> +    def test_analyzeTestReftestOneMatch(self):

No camelCase.

[Dirk Pranke]

Comment on attachment 201312 [details]
update w/ review feedback

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

>> Tools/Scripts/webkitpy/w3c/test_converter.py:41
>> +        self._webkit_root = __file__.split(self.filesystem.sep + 'Tools')[0]
> 
> Why aren't we using scm.checkout_root? We should at least have a FIXME.

Hm. There was a reason I didn't want to (or couldn't) use checkout_root here, but I'm forgetting what it is at the moment. I'll look into it again.

>>>> Tools/Scripts/webkitpy/w3c/test_converter.py:48
>>>> +        """ Reads the current list of CSS properties requiring the -webkit- prefix from Source/WebCore/CSS/CSSPropertyNames.in and stores them in an instance variable """
>>> 
>>> I would not put the full path, just CSSPropertyNames.in.
>> 
>> Sure, will fix.
> 
> Why do we need this comment?
> Why can't we rename the method to load_webkit_prefixed_css_property_list?
> 
> Also, we shouldn't abbreviate property as prop.

The comment is useful to indicate that we're getting the list from a particular filename, but I'll delete it since you seem so against it.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:76
>> +        # Handle plain old CSS files
> 
> If comment doesn't add any value.

will fix.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:118
>> +        converted_props = []
> 
> We should spell out properties.

will fix.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:136
>> +            scrubbed_style = self.scrub_unprefixed_props(style_text)
> 
> I would much prefer calling these as: add_webkit_prefix_to_unprefix_properties and prefixed_style_text or modified_style_text.

ok.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:150
>> +        """ Searches |text| for instances of properties requiring the -webkit- prefix and adds the prefix. Returns the list of converted properties and the modified string """
> 
> This method-level comment is redundant once we rename the method to add_webkit_prefix_to_unprefix_properties.

ok.

>> Tools/Scripts/webkitpy/w3c/test_converter.py:163
>> +                print 'converting ' + unprefixed_prop + ' -> ' + prefixed_prop
> 
> Do: "converting %s -> %s" % (unprefixed_prop, prefixed_prop)

will change, though I don't know why you think that's particularly better. not everything needs to be a format string.

>> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:45
>> +    def testLoadPrefixedPropList(self):
> 
> Why is this camelCase'd? And ditto for other tests.

will fix.

>> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:293
>> +                          'testharness href path was not converted')
> 
> Wrong indentation. Indent by 4 spaces.

will change, but this isn't the wrong indentation. This is legal PEP-8 (you and I have had this discussion before).

>> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:295
>> +        # Get the new relpath from the tester dir
> 
> This comment should be removed.

ok.

>> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:298
>> +        # Verify it's in all the right places
> 
> This comment doesn't add any value because "right places" are not obvious.

ok.

>> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:308
>> +        # Verify that the number of test properties equals the number that were converted
> 
> This comment repeats the code.

I believe these comments are useful, but fine ...

>> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:320
>> +        # Grab a random bunch of 20 unique properties requiring prefixes to test with
> 
> We shouldn't be randomly picking properties to test. r- because of this.

What would you prefer? the full list? A hard-coded subset?

>> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:323
>> +            idx = random.randint(0, len(full_prop_list) - 1)
> 
> What does idx mean?

index. I will spell it out, but that's a fairly common abbreviation.

>> Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:324
>> +            if not(full_prop_list[idx] in test_properties):
> 
> Why not "full_prop_list[idx] not in test_properties"

I will change it to that but I don't see that that is any better ...

>> Tools/Scripts/webkitpy/w3c/test_importer.py:130
>> +
> 
> Wrong indentations.

See comments above.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:178
>> +    def get_changeset(self):
> 
> This should be renamed to something like load_tip_changeset.
> Per style guideline, get_ prefix should be used for functions with an out argument.

Will change.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:188
>> +        """ Walks the |directory| looking for HTML files that are importable tests. """
> 
> Why can't we just rename this function find_importable_tests and get rid of the comment?

Will change.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:213
>> +                    continue  # For some reason the w3c repo contains random perl scripts we don't care about.
> 
> It seems like the whole block inside this for should be a separate function.

I noted this in my previous comments. The "continue"s in the loop make pulling this into a single function unwieldly, so I would prefer to leave as-is and potentially refactor later.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:274
>> +        """ Copies and converts the full list of importable tests into Webkit and logs what was imported in each directory """
> 
> I don't think this comment is useful because "importing tests" in this context implies all of that.

I don't believe that's implied. I'm leaving this as-is.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:332
>> +                    #       there's no harm in copying the identical thing.
> 
> s/TODO/FIXME/. Also comments shouldn't be indented inside like this.

Whoops, missed that one.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:334
>> +                    print '       As: ' + new_filepath
> 
> ":', " instead of ": ' +"

Ok.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:344
>> +                        shutil.copyfile(orig_filepath, new_filepath)  # The file was unmodified.
> 
> This comment repeats the code.

I don't think so. I'll leave the comment as is.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:381
>> +        """ Sets the test status to either 'approved' or 'submitted' """
> 
> I don't think this comment is useful.

I think it is.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:385
>> +        if self.source_directory.find('approved') != -1:
> 
> What happens if someone added a directory named preapproved? You probably need to filesystem.split and then find 'approved' token inside the list.

Will fix.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:398
>> +        if not(os.path.exists(import_log_file)):
> 
> No parenthesis for not.

Will fix.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:405
>> +            list_idx = contents.index('List of files:\n') + 1
> 
> We shouldn't abbreviate index as idx.

Will fix.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:407
>> +            previous_file_list = map(lambda s: s.strip(), previous_file_list)
> 
> Please don't use one letter variable like s. file?

Will fix. 'file' is a reserved word in python (or at least shadows a builtin function), but I'll pick something else.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:433
>> +        import_log.write('Properties requiring vendor prefixes:\n')
> 
> Why are we not using triple-quotes here?

I'm not a fan of triple quotes, and we have seveeral lines in here where values are getting interpolated. This is fine.

>> Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:41
>> +    def test_ImportDirWithNoTests(self):
> 
> No camelCase.

Will fix.

>> Tools/Scripts/webkitpy/w3c/test_parser.py:110
>> +        """ Searches for all reference links in the file, by looking for a rel of either "match" or "mismatch"."""
> 
> The method should be renamed to find_all_link_elements_with_rel, and the comment should be removed
> because rel can be anything, and the function is generic enough that it can return any element with rel attribute.
> I would much prefer for this function to return href of a link element instead though.
> Nevertheless, the comment is inaccurate.

The comment isn't inaccurate, it describes the legal values for the input parameter. You are getting the interface confused with the implementation (which happens to rely on the underlying rel= attribute matching). I will change the parameter name to something like 'reftest_type' to make that clearer.

>> Tools/Scripts/webkitpy/w3c/test_parser.py:114
>> +        """ Searches the file for usage of W3C-style testharness paths """
> 
> This comment should be removed.

I think it's useful to have a comment about how this is implemented. I will reword it slightly.

>> Tools/Scripts/webkitpy/w3c/test_parser.py:125
>> +        href_attrs = doc.findAll(href=re.compile('.*'))
> 
> These variables should be renamed to elements_with_src_attribute and elements_with_href_attribute

Will do.

[Dirk Pranke]

Comment on attachment 201312 [details]
update w/ review feedback

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

>>> Tools/Scripts/webkitpy/w3c/test_converter.py:150
>>> +        """ Searches |text| for instances of properties requiring the -webkit- prefix and adds the prefix. Returns the list of converted properties and the modified string """
>> 
>> This method-level comment is redundant once we rename the method to add_webkit_prefix_to_unprefix_properties.
> 
> ok.

Actually, I don't think it's redundant, since the return value is non-obvious.

>>> Tools/Scripts/webkitpy/w3c/test_importer.py:274
>>> +        """ Copies and converts the full list of importable tests into Webkit and logs what was imported in each directory """
>> 
>> I don't think this comment is useful because "importing tests" in this context implies all of that.
> 
> I don't believe that's implied. I'm leaving this as-is.

I take it back. Deleted.

>>> Tools/Scripts/webkitpy/w3c/test_parser.py:110
>>> +        """ Searches for all reference links in the file, by looking for a rel of either "match" or "mismatch"."""
>> 
>> The method should be renamed to find_all_link_elements_with_rel, and the comment should be removed
>> because rel can be anything, and the function is generic enough that it can return any element with rel attribute.
>> I would much prefer for this function to return href of a link element instead though.
>> Nevertheless, the comment is inaccurate.
> 
> The comment isn't inaccurate, it describes the legal values for the input parameter. You are getting the interface confused with the implementation (which happens to rely on the underlying rel= attribute matching). I will change the parameter name to something like 'reftest_type' to make that clearer.

Whoops. I misunderstood what this routine was doing. You are more correct than I. Will rework.

[Dirk Pranke]

Created attachment 202178 [details]
update w/ second round of review feedback

[WebKit Commit Bot]

Comment on attachment 202178 [details]
update w/ second round of review feedback

Clearing flags on attachment: 202178

Committed r150317: <http://trac.webkit.org/changeset/150317>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Comment on attachment 202178 [details]
update w/ second round of review feedback

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

> Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:51
> +            importer.do_import()

We can't do this. It tires to run hg.

[Ryosuke Niwa]

Disabled the test in http://trac.webkit.org/changeset/150324.

[Dirk Pranke]

(In reply to comment #55)
> Disabled the test in http://trac.webkit.org/changeset/150324.

It does try to run hg, but it's supposed to catch the exception if running it fails, so it should be fine. 

What was the problem you were seeing that caused you to actually disable the test? Was that logic not working?

[Ryosuke Niwa]

http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/10094

  Traceback (most recent call last):
    File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py", line 51, in test_import_dir_with_no_tests
      importer.do_import()
    File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/w3c/test_importer.py", line 170, in do_import
      self.load_changeset()
    File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/w3c/test_importer.py", line 178, in load_changeset
      self.changeset = self.host.executive.run_command(['hg', 'tip']).split('changeset:')[1]
    File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/common/system/executive.py", line 417, in run_command
      close_fds=self._should_close_fds())
    File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/common/system/executive.py", line 488, in popen
      return subprocess.Popen(string_args, **kwargs)
    File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 679, in __init__
      errread, errwrite)
    File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1228, in _execute_child
      raise child_exception
  OSError: [Errno 2] No such file or directory

[Dirk Pranke]

(In reply to comment #57)
> http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/10094
> 
>   Traceback (most recent call last):
>     File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py", line 51, in test_import_dir_with_no_tests
>       importer.do_import()
>     File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/w3c/test_importer.py", line 170, in do_import
>       self.load_changeset()
>     File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/w3c/test_importer.py", line 178, in load_changeset
>       self.changeset = self.host.executive.run_command(['hg', 'tip']).split('changeset:')[1]
>     File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/common/system/executive.py", line 417, in run_command
>       close_fds=self._should_close_fds())
>     File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/common/system/executive.py", line 488, in popen
>       return subprocess.Popen(string_args, **kwargs)
>     File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 679, in __init__
>       errread, errwrite)
>     File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1228, in _execute_child
>       raise child_exception
>   OSError: [Errno 2] No such file or directory

Ah. I wonder why that's not coming back as a ScriptError. Will fix, thanks!