Bug 76749

Summary: run-webkit-tests --lint-test-files should lint all the ports by default
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 76745    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
don't change log messages, update w/ review feedback
none
add tests ojan: review+

Description Dirk Pranke 2012-01-20 15:37:49 PST
run-webkit-tests --lint-test-files should lint all the ports by default
Comment 1 Dirk Pranke 2012-01-20 15:41:20 PST
Created attachment 123397 [details]
Patch
Comment 2 Ojan Vafai 2012-01-20 15:54:39 PST
Comment on attachment 123397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123397&action=review

Can you add a simple test that verifies that we actually iterate through all the test configurations? Something like http://trac.webkit.org/changeset/105452.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:53
> +        ports_to_lint = [host.port_factory.get(name) for name in host.port_factory.all_port_names()]

This is going to be slow. We'll be checking the each Chromium port when we only need to check one of them, no?

I'm not sure what the right design is here, but it would be good to do some quick performance testing to see what the impact is once bug 76745 is resolved.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:70
> +    # Currently all real ports use the same list of tests and then skip
> +    # tests as desired per platform. So, in order to speed this up, we
> +    # only fetch the list of tests once, instead of once per port as would
> +    # be strictly correct.
> +    # FIXME(dpranke): Should we cache this list in port/base.py instead?
> +    #
> +    # The exception is the Chromium GPU tests, but
> +    # (a) they're going away, and (b) they're a subset of the Chromium tests,
> +    # so it should be safe to reuse the larger list with them.
> +    printer.print_update("Collecting tests ...")
> +    try:
> +        tests = port.tests([])
> +    except IOError, e:
> +        if e.errno == errno.ENOENT:
> +            return -1
> +        raise

I don't think we need to pass the list of tests at all.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:73
> +    for port_to_lint in ports_to_lint:

Port<-->test_configuration is a 1:N mapping. You need to call all_test_configurations() for each port.
Comment 3 Dirk Pranke 2012-01-20 15:59:20 PST
Hm, actually after talking to Dimitri and looking at bug 76745, the approach is probably overkill and we should only lint one port per test_expectations_file since the expectations parser now catches errors across all test configurations of the port, not just the one.
Comment 4 Dirk Pranke 2012-01-20 16:01:55 PST
(In reply to comment #2)
> (From update of attachment 123397 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123397&action=review
> 
> Can you add a simple test that verifies that we actually iterate through all the test configurations? Something like http://trac.webkit.org/changeset/105452.
> 

Sure.

> > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:53
> > +        ports_to_lint = [host.port_factory.get(name) for name in host.port_factory.all_port_names()]
> 
> This is going to be slow. We'll be checking the each Chromium port when we only need to check one of them, no?
>

I thought that we actually had to test each port, but it turns out that I didn't understand how the test expecations parser had changed to look at all configurations at once. So, yeah, I think we only need to use one port per file.
 
> I'm not sure what the right design is here, but it would be good to do some quick performance testing to see what the impact is once bug 76745 is resolved.
>

Agreed. I will wait until that change lands and revisit.
 
> I don't think we need to pass the list of tests at all.
>

After thinking about this further, I think this is right solely for purposes of find errors in the file. If you actually needed to know the expectations for a given test (which you don't, for --lint-test-files), I think you do.
 
> > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:73
> > +    for port_to_lint in ports_to_lint:
> 
> Port<-->test_configuration is a 1:N mapping. You need to call all_test_configurations() for each port.

True, but as discussed above, I think this is moot now.
Comment 5 Dirk Pranke 2012-01-20 16:34:42 PST
also, I'm just gonna wait until Ojan stops hacking on this stuff before I start again. Too many cooks in one pot :).
Comment 6 Dirk Pranke 2012-01-24 13:28:17 PST
Created attachment 123794 [details]
Patch
Comment 7 Dirk Pranke 2012-01-24 13:29:57 PST
Updated after Ojan's fix to speed up --lint-test-files. It now takes 1.5 seconds to lint the chromium file and 4.5 seconds to lint all of the files on my Mac Pro.

This change also cleans up the output a bit to be more "quickfix" friendly and to make it clearer which errors are in which files.
Comment 8 Ojan Vafai 2012-01-24 14:03:31 PST
Comment on attachment 123794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123794&action=review

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:50
> +        # Only lint the platform the user asked for.

This comment is not helpful.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:68
> +                None,  # tests
> +                port_to_lint.test_expectations(),
> +                port_to_lint.test_configuration(),
> +                True,  # is_lint_mode

Can you use named arguments here instead of comments?

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:99
> +    if options.lint_test_files:
> +        return lint(port, options)

You missed deleting the manager.lint() call below.
Comment 9 Ojan Vafai 2012-01-24 14:04:07 PST
Comment on attachment 123794 [details]
Patch

Whoops. R+'d a bit preemptively. Can you add tests?
Comment 10 Dirk Pranke 2012-01-24 14:20:31 PST
(In reply to comment #8)
> (From update of attachment 123794 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123794&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:50
> > +        # Only lint the platform the user asked for.
> 
> This comment is not helpful.
> 

Ok, will delete.

> > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:68
> > +                None,  # tests
> > +                port_to_lint.test_expectations(),
> > +                port_to_lint.test_configuration(),
> > +                True,  # is_lint_mode
> 
> Can you use named arguments here instead of comments?
> 

Sure.

> > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:99
> > +    if options.lint_test_files:
> > +        return lint(port, options)
> 
> You missed deleting the manager.lint() call below.

Will fix.

(In reply to comment #9)
> (From update of attachment 123794 [details])
> Whoops. R+'d a bit preemptively. Can you add tests?

That's okay, I posted it for review a bit preemptively as well, since a bunch of existing tests fail :). Will fix and add tests.
Comment 11 Dirk Pranke 2012-01-24 14:29:47 PST
Created attachment 123811 [details]
don't change log messages, update w/ review feedback
Comment 12 Dirk Pranke 2012-01-24 15:07:14 PST
Created attachment 123821 [details]
add tests
Comment 13 Dirk Pranke 2012-01-24 15:09:36 PST
Please take another look?

On a related note, I've realized that a bot should only lint its own file; it would be really annoying if a Chromium bot went red because of an error in the Apple expectations file that was otherwise unused.

I still think that when run interactively with no other arguments, linting all of the bots the right thing to do (to ensure that chromium is linted by default).
Comment 14 Dirk Pranke 2012-01-25 17:33:27 PST
This landed as r105936; I'm not sure why the bug didn't get closed.