Bug 49835 – Ignore reference files which will be used by reftests when collecting test cases.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Hayato Ito]

This is a necessary step to support reftests. We need to ignore reference files when collecting test files in NRWT.

[Hayato Ito]

Created attachment 74425 [details]
ignore-ref-files

[WebKit Review Bot]

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.

[Hayato Ito]

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

[Shinichiro Hamaji]

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.

[Dirk Pranke]

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."

[Hayato Ito]

Created attachment 74594 [details]
ignore-reffiles-3

[Hayato Ito]

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.

[Dirk Pranke]

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"

[Hayato Ito]

Created attachment 74597 [details]
ignore-reffiles-4

[Hayato Ito]

(In reply to comment #8)
> 
> Nit: typo - "poinsts"

I am sorry, fixed.

[Hayato Ito]

Comment on attachment 74597 [details]
ignore-reffiles-4

Clearing flags on attachment: 74597

Committed r72564: <http://trac.webkit.org/changeset/72564>

[Hayato Ito]

All reviewed patches have been landed.  Closing bug.