[webkit-reviews] review denied: [Bug 111513] Create a script to import W3C tests : [Attachment 201312] update w/ review feedback

Fri May 17 16:17:15 PDT 2013

Ryosuke Niwa <rniwa at webkit.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 111513: Create a script to import W3C tests
https://bugs.webkit.org/show_bug.cgi?id=111513

Attachment 201312: update w/ review feedback
https://bugs.webkit.org/attachment.cgi?id=201312&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.