Bug 48878 – NRWT doesn't have good test coverage for --run-chunk, --batch-size, --run-part, etc.

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

[Mihai Parparita]

NRWT doesn't have good test coverage for --run-chunk, --batch-size, --run-part, etc.

[Mihai Parparita]

Created attachment 72751 [details]
Patch

[Mihai Parparita]

I'd like to add support to NRWT for --iterations (which is useful for tracking down flaky tests), and it seemed like having better test coverage was a good idea before I messed with TestRunner too much.

[Dirk Pranke]

Comment on attachment 72751 [details]
Patch

I think it would be better if we were using the regular output for this, rather than creating a mock port. Tests requiring mock ports will break when we switch to a multi-process version of NRWT.

You can get mostly what you need out of --print trace-everything, but I've been thinking it would be better if we had a one-line-per-test terser version of that for the testing I've been doing recently myself.

[Dirk Pranke]

Okay, from further discussion w/ mihai , and to clarify ...

Generally speaking, we don't want to rely on MockPorts because they (probably) won't work in a multi-process scenario, and I don't want to write too many tests that rely on single-process testing.

That said, there's no obvious way to test the --batch-size argument without this (we'd have to parse log output, or modify the code to expose things I don't really want to expose), so I'm okay with using a mock port for this.

--run-chunk and --run-part are a different story. Those should be black-box testable. We should probably modify run_webkit_tests.run to return more detailed output over what we're actually doing in the test, and that would be a good way to test that.

But, since that code doesn't exist now, and since you have to write the port fixture to test --batch-size anyway, I'm fine with reusing that code for testing --run-chunk and --run-part for now.

So, the patch looks good to me.

[Tony Chang]

Comment on attachment 72751 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:109
> +def get_tests_run(args=[], tests_included=False, flatten_batches=False):

Nit: The use of args=[] as a default arg is correct in this case only because we never modify args in the function.  I would just remove the default value so it doesn't trip someone up in the future.

[Eric Seidel (no email)]

Comment on attachment 72751 [details]
Patch

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

Looks good.  THanks!

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:99
> +    if port_obj is None:

Why not just "not port_obj".  Maybe is None is google style.  It's more specific, just not sure the extra specificity buys us much.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:110
> +    new_args = [

What's new about these args? :)

[Mihai Parparita]

Created attachment 72782 [details]
Patch

[Mihai Parparita]

For all these changes, since get_tests_run was mirroring the existing passing_run/logging_run functions, I changed all three to be consistent.

(In reply to comment #5)
> Nit: The use of args=[] as a default arg is correct in this case only because we never modify args in the function.  I would just remove the default value so it doesn't trip someone up in the future.

We do omit the argument occasionally though. I changed the default to None. I also renamed it to extra_args to make its use clearer.


(In reply to comment #6)
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:99
> > +    if port_obj is None:
> 
> Why not just "not port_obj".  Maybe is None is google style.  It's more specific, just not sure the extra specificity buys us much.

I like "not port_obj", switched to that.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:110
> > +    new_args = [
> 
> What's new about these args? :)

Renamed this to args (and the passed in list to extra_args).

[Dirk Pranke]

(In reply to comment #8)
> For all these changes, since get_tests_run was mirroring the existing passing_run/logging_run functions, I changed all three to be consistent.
> 
> (In reply to comment #5)
> > Nit: The use of args=[] as a default arg is correct in this case only because we never modify args in the function.  I would just remove the default value so it doesn't trip someone up in the future.
> 
> We do omit the argument occasionally though. I changed the default to None. I also renamed it to extra_args to make its use clearer.
>

Hm. That actually makes it less clear to me than before, but perhaps that's not surprising, since I was the one that called it new_args.

The original idea was that the caller would pass in an argument list. The function would clone the list and modify it (hence, new_args). Over time, given, the way that code has evolved, that logic was probably no longer clear.

That said, I would be a little concerned that passing_run() and its kin always modify the list to pass at least some arguments, so there is no longer a way to truly get the original intent of the function. Maybe that's not really needed and it's okay the way it is ...
 
> > 
> > Why not just "not port_obj".  Maybe is None is google style.  It's more specific, just not sure the extra specificity buys us much.
> 
> I like "not port_obj", switched to that.
>

Changing to "not port_obj" actually prevents a class of possible errors (e.g., port_obj=[]) that would have occurred before.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 72782 [details]:

http/tests/loading/bad-server-subframe.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 72782 [details]
Patch

Clearing flags on attachment: 72782

Committed r71359: <http://trac.webkit.org/changeset/71359>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

(In reply to comment #11)
> (From update of attachment 72782 [details])
> Clearing flags on attachment: 72782
> 
> Committed r71359: <http://trac.webkit.org/changeset/71359>

Some python tests are failing since this change on Leopard and Qt Linux.
http://build.webkit.org/builders/Leopard%20Intel%20Release%20(Tests)/builds/23760/steps/webkitpy-test/logs/stdio

[Mihai Parparita]

(In reply to comment #13)
> (In reply to comment #11)
> > (From update of attachment 72782 [details] [details])
> > Clearing flags on attachment: 72782
> > 
> > Committed r71359: <http://trac.webkit.org/changeset/71359>
> 
> Some python tests are failing since this change on Leopard and Qt Linux.
> http://build.webkit.org/builders/Leopard%20Intel%20Release%20(Tests)/builds/23760/steps/webkitpy-test/logs/stdio

Looks like itertools.chain.from_iterable was added in 2.6, I guess we're running with 2.5 there. I think I can just use itertools.chain directly, which should work everywhere (will work on a patch).

[Eric Seidel (no email)]

Yes, webkitpy is supposed to be 2.5+.

[Mihai Parparita]

(In reply to comment #14)
> (will work on a patch).

Patch is up at bug 49043.

[Mihai Parparita]

(In reply to comment #15)
> Yes, webkitpy is supposed to be 2.5+.

Looks like the cq uses 2.6, otherwise this wouldn't have landed.

[Kent Tamura]

(In reply to comment #16)
> Patch is up at bug 49043.

Thanks!