NRWT doesn't have good test coverage for --run-chunk, --batch-size, --run-part, etc.
Created attachment 72751 [details] Patch
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.
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.
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.
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.
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? :)
Created attachment 72782 [details] Patch
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).
(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.
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 mjs@apple.com. The commit-queue is continuing to process your patch.
Comment on attachment 72782 [details] Patch Clearing flags on attachment: 72782 Committed r71359: <http://trac.webkit.org/changeset/71359>
All reviewed patches have been landed. Closing bug.
(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
(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).
Yes, webkitpy is supposed to be 2.5+.
(In reply to comment #14) > (will work on a patch). Patch is up at bug 49043.
(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.
(In reply to comment #16) > Patch is up at bug 49043. Thanks!