[webkit-reviews] review denied: [Bug 49773] nrwt multiprocessing - clean up test sharding a bit, add --workers-are flag : [Attachment 74453] merge to tip of tree (r72458)

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 49773: nrwt multiprocessing - clean up test sharding a bit, add
--workers-are flag
https://bugs.webkit.org/show_bug.cgi?id=49773

Attachment 74453: merge to tip of tree (r72458)
https://bugs.webkit.org/attachment.cgi?id=74453&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
r- only for not validating possible values of workers-are.  The rest is just
bikeshedding about naming, but I think we can come up with something better
than workers_are.

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

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:498
> +    def _shard_tests(self, test_files, single_threaded):

Maybe single_group instead of single_threaded since this is True if
experiment_fully_parallel is true?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:586
> -	   """Returns whether we should run all the tests in the main
thread."""
> +	   """Returns whether to pluralize log messages about DRT instances."""


The new docstring is confusing to me.  I'm not sure this function even needs a
docstring.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1591
> +	   optparse.make_option("--workers-are", action="store",
> +				default="threads",
> +				help="controls worker model. Valid values are "

> +				"'inline' and 'threads' (default)."),

As mentioned in bug 49779, I think workers-are is a weird variable name.  I
think concurrency-{mode,type,model} would be more descriptive, but hard to type
as a command line flag.  Maybe --concurrency=inline (or single) and
--concurrency=threads isn't too bad. The variable could still be
concurrency_mode.  Maybe worker_mode, but that's pretty vague too.

To be clear, this means you could have flags for inline and
experimental-fully-parallel?  Maybe experiment-fully-parallel should be rolled
into this enum to avoid conflicting flags (separate change, of course)?

Also, can we check this somewhere (i.e., raise an exception if it's not inline
or threads)?  It looks like you can add custom types to optparse, but that
might not be worth it.