Bug 49835 - Ignore reference files which will be used by reftests when collecting test cases.
Summary: Ignore reference files which will be used by reftests when collecting test ca...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 36065
  Show dependency treegraph
 
Reported: 2010-11-19 14:32 PST by Hayato Ito
Modified: 2010-11-22 14:52 PST (History)
4 users (show)

See Also:


Attachments
ignore-ref-files (3.98 KB, patch)
2010-11-19 14:37 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
ignore-ref-files-fix-style (3.98 KB, patch)
2010-11-19 14:44 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
ignore-reffiles-3 (3.55 KB, patch)
2010-11-22 13:51 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
ignore-reffiles-4 (3.54 KB, patch)
2010-11-22 14:19 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2010-11-19 14:32:59 PST
This is a necessary step to support reftests. We need to ignore reference files when collecting test files in NRWT.
Comment 1 Hayato Ito 2010-11-19 14:37:19 PST
Created attachment 74425 [details]
ignore-ref-files
Comment 2 WebKit Review Bot 2010-11-19 14:40:36 PST
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.
Comment 3 Hayato Ito 2010-11-19 14:44:04 PST
Created attachment 74426 [details]
ignore-ref-files-fix-style
Comment 4 Shinichiro Hamaji 2010-11-19 15:17:44 PST
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 5 Dirk Pranke 2010-11-19 17:25:03 PST
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."
Comment 6 Hayato Ito 2010-11-22 13:51:39 PST
Created attachment 74594 [details]
ignore-reffiles-3
Comment 7 Hayato Ito 2010-11-22 13:57:19 PST
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 8 Dirk Pranke 2010-11-22 14:09:26 PST
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"
Comment 9 Hayato Ito 2010-11-22 14:19:33 PST
Created attachment 74597 [details]
ignore-reffiles-4
Comment 10 Hayato Ito 2010-11-22 14:20:19 PST
(In reply to comment #8)
> 
> Nit: typo - "poinsts"

I am sorry, fixed.
Comment 11 Hayato Ito 2010-11-22 14:52:43 PST
Comment on attachment 74597 [details]
ignore-reffiles-4

Clearing flags on attachment: 74597

Committed r72564: <http://trac.webkit.org/changeset/72564>
Comment 12 Hayato Ito 2010-11-22 14:52:51 PST
All reviewed patches have been landed.  Closing bug.