-
Couldn't load subscription status.
- Fork 292
Add configuration option for test executor arguments #2636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
docs/options.md
Outdated
| ### `test-execution-args` {: #test-execution-args toml env-var } | ||
|
|
||
| > Define additional arguments that will be passed to the command that runs the tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test-execution-args still sounds like they might be arguments to the tests themselves. Would test-runner-args be a clearer name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I picked test-execution-args based on the discussion that lead to this PR; but test-runner-args would also work. The exact naming isn't a hill I'll die on, and it's easy to change; would be interested in hearing other opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had considered test-runner-... too, but I didn't like it because to my mind 'pytest' is a test runner, and we're not talking about that. Ideally, the name would evoke more 'environment' or 'context' in its flavour, but nothing fit that didn't already have a meaning. So I landed on 'execution', which feels higher-level than runner(?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some other options:
- "testbed" is the term we've used for both Android and iOS, but that wouldn't be appropriate for a Docker container, as mentioned in the linked discussion.
- "container" is what Docker would call it, but that implies a certain level of isolation which we don't necessarily provide.
- "launcher" seems like a fairly generic way of saying "the thing that starts (but doesn't "run") the test suite".
On the other hand, if this option could have additional sub-keys in the future as suggested below, then maybe a general name like "execution" is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if I'm completely honest, I'm still not in love with 'test-execution'. Seeing it in the sidebar, it really gives no hints about what it does-
I'm now coming back around to test-target, test-host or test-runtime. Actually test-target looks quite sensible in the options sidebar-
I'm curious what resonates with you? bearing in mind I could also imagine this being used to spec a docker image, or set some pyodide/node options.
(apologies to go around the houses with this @freakboy3742 !)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious what resonates with you? bearing in mind I could also imagine this being used to spec a docker image, or set some pyodide/node options.
Of those options, test-runtime makes the most sense to me.
test-target sounds like "where do I want to run the tests?" rather than "how will I run this test on a target?" (and similar for test-host, with the added benefit of the confusion of whether the "host" is the architecture running the build, or the architecture you're building for).test-runtime sounds like the one that is most directly tied to "what runtime will I use for the tests?" - which is essentially what we're configuring, whether that's iOS, Android or Docker.
(apologies to go around the houses with this @freakboy3742 !)
No problems at all - it's an easy set of changes to make, and I'd rather go round a few times now and get it right than have to walk back a change in a couple of weeks/months.
.github/workflows/test.yml
Outdated
| # has problems using any simulator that isn't pre-warmed. | ||
| # Unfortunately, the simulator that it picks by default isn't | ||
| # pre-warmed. So - we need to explicitly select a simulator. | ||
| test_execution_args: '--simulator "iPhone 16e,OS=18.5"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this the default, at least when running on GitHub Actions, so each project doesn't need to specify it? Experience so far has shown that every required bit of manual configuration will trip up a significant number of package developers, even if the documentation is clear enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the "working default" can (and based on recent evidence, does) change somewhat arbitrarily. If a value is hard coded into cibuildwheel, then it needs a cibuildwheel release to fix it.
Even the fact that an override is needed at all is a temporary measure - once GitHub/Apple resolve this hypervisor performance issue, there presumably won't be a need for an override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative idea – though I guess this might need to be done on the CPython side – if we need to use a simulator which is warmed up, can we detect which simulators are warmed up and use them by default? That would give better performance even after the current issue is fixed,
Again, this could be a CI-specific behavior, but there is precedent for doing that to work around issues which are likely to keep happening, such as the Android emulator permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't be opposed to putting some CI-specific code into cibuildwheel to smooth over temporary issues like this, especially if there's a user-controllable escape hatch to override our probably-helpful defaults.
We'll need to adapt to CI images for the sake of our CI anyway, so might as well roll that into the product and let our users benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we detect which simulators are warmed up and use them by default?
The only suggestion I've seen to date involves a command that "succeeds on warm simulators, hangs on non-warm simulators".
I wouldn't be opposed to putting some CI-specific code into cibuildwheel to smooth over temporary issues like this, especially if there's a user-controllable escape hatch to override our probably-helpful defaults.
Adding a (currently known to work) default value when on GitHub Actions + macOS 15 is certainly a possibility, and I agree that it would definitely be preferable for this to work out of the box without configuration. As I mentioned previously - my hesitation is that we don't know what Github will do to fix the problem, and on what timeframe; and once it's baked into cibuildwheel itself, it takes a full release cycle to "unbake" it.
I guess taking a similar approach to Android (i.e., "here's a default; if you provide something manually we'll ignore that default") is an option, as it at least provides a window for an immediate workaround if it turns out the update is incompatible. I'll take a look to see if I can make that work.
Co-authored-by: Malcolm Smith <[email protected]>
…the default args to the test runner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work on this! It looks good. A few comments-
docs/options.md
Outdated
| ### `test-execution-args` {: #test-execution-args toml env-var } | ||
|
|
||
| > Define additional arguments that will be passed to the command that runs the tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had considered test-runner-... too, but I didn't like it because to my mind 'pytest' is a test runner, and we're not talking about that. Ideally, the name would evoke more 'environment' or 'context' in its flavour, but nothing fit that didn't already have a meaning. So I landed on 'execution', which feels higher-level than runner(?).
.github/workflows/test.yml
Outdated
| # has problems using any simulator that isn't pre-warmed. | ||
| # Unfortunately, the simulator that it picks by default isn't | ||
| # pre-warmed. So - we need to explicitly select a simulator. | ||
| test_execution_args: '--simulator "iPhone 16e,OS=18.5"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't be opposed to putting some CI-specific code into cibuildwheel to smooth over temporary issues like this, especially if there's a user-controllable escape hatch to override our probably-helpful defaults.
We'll need to adapt to CI images for the sake of our CI anyway, so might as well roll that into the product and let our users benefit.
|
@joerick I've just pushed an update that includes:
I've kept the explicit use of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these latest changes - just a couple things...
| test_execution_args = [ | ||
| "--simulator", | ||
| "iPhone 16e,OS=18.5", | ||
| *test_execution_args, | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if the user could pass test-execution = "args: --simulator auto" to activate the select_simulator_device(platform) function from the test script. But I suppose that's a CPython change. Not essential, this is okay, it's probably a temporary issues anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - adding auto would require an upstream CPython change; and "auto" is already the default behavior for the iOS test runner.
This is only needed as a workaround for the current set of GitHub Actions weirdness - and the 2 month release cadence of CPython definitely doesn't lend itself to hard-coding defaults like this.
Co-authored-by: Joe Rickerby <[email protected]>
iOS and Android test suites are executed using a testbed app. This app needs to be started, and the test arguments (the value of
test_command) is passed to that testbed.However, the testbed app itself has configuration arguments, and it can sometimes be useful to be able to use these arguments.
This issue has become mildly critical as a result of the recent changes to the
macos-15test runner. At present, the simulator that is picked by default by the iOS test runner is entirely valid from the perspective of Xcode, but then refuses to launch.This is a known issue with the macOS-15 runner that GitHub is investigating; but it's not a once-off issue. When Xcode 16.4 was released, it removed the "iPhone SE" simulator as a default image, and as there was no way to configure the simulator used at runtime, a full update of the iOS support packages and a cibuildwheel release was necessary to resolve the issue.
So - this PR adds a
CIBW_TEST_EXECUTION_ARGSsetting (with related platform-specific alternatives). These arguments are passed to the testbed runner on iOS and Android.As Android requires explicitly specifying the simulator to use as part of the runner arguments, the value of
CIBW_TEST_EXECUTION_ARGSis lightly processed. If there's a--managedor--connectedvalue inCIBW_TEST_EXECUTION_ARGS, that value will be used by the testbed runner; if no such argument exists,--managed maxVersionwill be used by the test runner.I'm not entirely sure how best to test this; it's in a portion of the code that can't be easily tested without actually starting simulators, and... well... the problem that led to this PR is that we can't use other simulators :-) So - I've gone with testing this empirically through the Github Actions configuration, passing in the necessary arguments so that on GitHub, simulators are specified on iOS and Android x86 simulator. On other platforms, the default behavior will continue to operate.