[Webkit-unassigned] [Bug 111513] Create a script to import W3C tests

Fri Mar 8 10:21:35 PST 2013

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

--- Comment #11 from Mihai Balan <mibalan at adobe.com>  2013-03-08 10:24:00 PST ---
(From update of attachment 192111)
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.

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