Bug 76749 – run-webkit-tests --lint-test-files should lint all the ports by default

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

run-webkit-tests --lint-test-files should lint all the ports by default

[Dirk Pranke]

Created attachment 123397 [details]
Patch

[Ojan Vafai]

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.

[Dirk Pranke]

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.

[Dirk Pranke]

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

[Dirk Pranke]

also, I'm just gonna wait until Ojan stops hacking on this stuff before I start again. Too many cooks in one pot :).

[Dirk Pranke]

Created attachment 123794 [details]
Patch

[Dirk Pranke]

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.

[Ojan Vafai]

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.

[Ojan Vafai]

Comment on attachment 123794 [details]
Patch

Whoops. R+'d a bit preemptively. Can you add tests?

[Dirk Pranke]

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

[Dirk Pranke]

Created attachment 123811 [details]
don't change log messages, update w/ review feedback

[Dirk Pranke]

Created attachment 123821 [details]
add tests

[Dirk Pranke]

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

[Dirk Pranke]

This landed as r105936; I'm not sure why the bug didn't get closed.