Expose the manager_number to the Browser class itself #52455
Conversation
wpt-pr-bot
added
infra
wptrunner
|
…and I managed to break the tests while tidying this up to post as a PR, and clearly forgot to run them. Doh. |
There was a problem hiding this comment.
This is at least an improvement.
To make this patch work for Chromium on Android, please apply jonathan-j-lee/wpt@44a7818, which I've tested downstream. Thanks!
| disable_fission=False, stackfix_dir=None, leak_check=False, | ||
| asan=False, chaos_mode_flags=None, config=None, | ||
| browser_channel="nightly", headless=None, debug_test=None, | ||
| binary=None, package_name="org.mozilla.geckoview.test_runner", device_serial=None, |
There was a problem hiding this comment.
question: I take it we can remove the package_name="org.mozilla.geckoview.test_runner" default because it had no effect in practice? i.e., browser_kwargs always plumbs in wpt run --package-name, defaulting to None.
There was a problem hiding this comment.
I was gonna say Mozilla's own CI uses wptrunner as a library in its own right, not via wpt run.
But we invoke browser_kwargs itself, so maybe that's pointless.
There was a problem hiding this comment.
The underlying change here seems good.
I don't love using **kwargs in more places; it makes it harder to add types in the future (see also the use of **kwargs: Any in this patch), and also makes it less clear which arguments are actually required for the browser classes to work as expected, and affects the ability of tooling like LSPs to help by providing the function signature.
|
IMO, the broader problems about passing arguments to superclasses is a general problem with typing in Python, and explicitly listing what you believe to be all superclass arguments is endlessly fragile. There's no good solution, and it really need to be sorting at the Python typing level… |
|
Adding more logging shows A load of these only apply to |
gsnedders
force-pushed
the
manager_number
branch
3 times, most recently
from
79e65a0 to
c6f4755
Compare
The goal here is for browser classes to be able to have the ability to get at data items specific to the current thread (e.g., with --processes=X, each of the X concurrently existing Browser objects should be able to get their own kwargs). This avoids needing to hardcode specific browser settings in wptrunner.testrunner, which is a layering violation. Adding a required argument to wptrunner.browsers.base.Browser obviously carries a risk, as shown by the number of browsers which needed modified to correctly pass through unknown keyword arguments to their parents, but I don't think there's a reasonable alternative to doing this. Co-authored-by: Jonathan Lee <jonathanjlee@google.com>
1. The default subsuite is the empty string, which, when logged, looks
incomplete (e.g., `INFO Restarting browser for new subsuite:`).
Format with `repr()` to add quotation marks.
2. Don't pass `debug_info` as a `HeadlessShellBrowser` kwarg. It gets
logged as `WARNING Browser.__init__ kwargs: {'debug_info': None}`
because it's unused (see web-platform-tests#52455).
... by using the `chrome` browser kwargs (the browser implementations happen to be identical at this time). This also fixes an unused `debug_info` kwarg, which after web-platform-tests#52455, gets logged as: ``` WARNING Browser.__init__ kwargs: {'debug_info': None} ```
... by using the `chrome` browser kwargs (the browser implementations happen to be identical at this time). This also fixes an unused `debug_info` kwarg, which after #52455, gets logged as: ``` WARNING Browser.__init__ kwargs: {'debug_info': None} ```
6 participants
The goal here is for browser classes to be able to have the ability to get at data items specific to the current thread (e.g., with
--processes=X, each of theXconcurrently existingBrowserobjects should be able to get their ownkwargs).This avoids needing to hardcode specific browser settings in
wptrunner.testrunner, which is a layering violation.Adding a required argument to
wptrunner.browsers.base.Browserobviously carries a risk, as shown by the number of browsers which needed modified to correctly pass through unknown keyword arguments to their parents, but I don't think there's a reasonable alternative to doing this.The only other real alternative would be to add
manager_numberas an argument toget_browser_kwargsandget_executor_kwargs, and turn them into partials, and evaluate them close to when we actually need them — when we would actually know what thread we're running in, and can thus supply that data to the getter functions in general.The biggest risk here is probably with
wptrunner.browsers.chrome_android.ChromeAndroidBrowserBaseand its subclasses, because that's neither tested in CI nor do I have an easy way to test that locally.