Bug 49835

Created attachment 74425 [details] ignore-ref-files

Attachment 74425 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKitTools/ChangeLog', u'WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py', u'WebKitTools/Scripts/webkitpy/layout_tests/port/test_files_unittest.py']" exit_code: 1 WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:124: indentation is not a multiple of four [pep8/E111] [5] WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:125: indentation is not a multiple of four [pep8/E111] [5] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 74426 [details] ignore-ref-files-fix-style

Comment on attachment 74426 [details] ignore-ref-files-fix-style View in context: https://bugs.webkit.org/attachment.cgi?id=74426&action=review Style nitpicks. > WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:122 > + if any(filename.endswith(ignored_suffix) for ignored_suffix I think a simple loop would be easier to read and not longer than this code: for ignored_suffix in _ignored_suffixes: if filename.endwith(ignored_suffix): _log.warn(... return True return False > WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:124 > + _log.warn('Reftests is not suuperted yet. %s is ignored.', filename) "Reftest is not" ? > WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:130 > + """Return true if filename represents test cases we want to run a test It would be better to have one-line summary on the top of docstring.

Comment on attachment 74426 [details] ignore-ref-files-fix-style View in context: https://bugs.webkit.org/attachment.cgi?id=74426&action=review > WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:123 > + in _ignored_suffixes): I actually think the any() is fine, but the variable names make it a bit hard to read. I'd use: if any(filename.endswith(suffix) for suffix in _ignored_suffixes): _log.warn(...) return True return False I would also consider renaming is_ignored_file to is_reftest and moving the _ignored_suffixes constant inside the function. You could get away with: def _is_reftest(filename): return (filename.endswith('-expected.html') or filename.endswith('-expected-mismatch.html')) Slightly less generic, but a lot easier to read and understand. >> WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:124 >> + _log.warn('Reftests is not suuperted yet. %s is ignored.', filename) > > "Reftest is not" ? _log.warn("Ignoring %s - reftests are not supported." % filename) or _log.warn("Reftests are not supported - ignoring %s" % filename) depending on which aspect you want to emphasize. > WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:131 > + on.""" I'm not sure that a docstring really adds anything here, but if you really wanted one, you could have "Return True if the filename points to a test file."

Created attachment 74594 [details] ignore-reffiles-3

Thank you for the reviews, hamaji-san and Dirk. (In reply to comment #5) > (From update of attachment 74426 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74426&action=review > > > WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:123 > > + in _ignored_suffixes): > > I actually think the any() is fine, but the variable names make it a bit hard to read. I'd use: > > if any(filename.endswith(suffix) for suffix in _ignored_suffixes): > _log.warn(...) > return True > return False > > I would also consider renaming is_ignored_file to is_reftest and moving the _ignored_suffixes constant inside the function. > > You could get away with: > > def _is_reftest(filename): > return (filename.endswith('-expected.html') or > filename.endswith('-expected-mismatch.html')) > > Slightly less generic, but a lot easier to read and understand. I agree. I just renamed that function to 'is_reference_html_file'. I think that might be better. It seems I did a too early generalization in the previous patch. > > >> WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:124 > >> + _log.warn('Reftests is not suuperted yet. %s is ignored.', filename) > > > > "Reftest is not" ? > > _log.warn("Ignoring %s - reftests are not supported." % filename) > > or > > _log.warn("Reftests are not supported - ignoring %s" % filename) > > depending on which aspect you want to emphasize. Thank you. Done. > > > WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:131 > > + on.""" > > I'm not sure that a docstring really adds anything here, but if you really wanted one, you could have "Return True if the filename points to a test file." Done.

Comment on attachment 74594 [details] ignore-reffiles-3 View in context: https://bugs.webkit.org/attachment.cgi?id=74594&action=review LGTM apart from the typo :) > WebKitTools/Scripts/webkitpy/layout_tests/port/test_files.py:117 > + """Return true if the filename poinsts to a reference HTML file.""" Nit: typo - "poinsts"

Created attachment 74597 [details] ignore-reffiles-4

(In reply to comment #8) > > Nit: typo - "poinsts" I am sorry, fixed.

Comment on attachment 74597 [details] ignore-reffiles-4 Clearing flags on attachment: 74597 Committed r72564: <http://trac.webkit.org/changeset/72564>

All reviewed patches have been landed. Closing bug.