Change to the iOS testing option semantics

[joerick]

cc @freakboy3742

I've been thinking about the test-command option, and the implicit addition of python -m.

• I was thinking that the failure mode of any command in here that isn't pytest is gonna be pretty confusing for users. e.g. test-command = "python ./my-tests.py" -> python: No module named python
• it's a bit awkward that if a user's test command really is python -m mymodule.tests they have to define it twice, once as-is and again for iOS as mymodule.tests

My thought was that we could require test commands on ios start with python -m, but with a warning and an auto-fixup where that doesn't occur. That way bad configs always get a warning, and we allow more option reuse between platforms.

I'd want to add a test for this, so just raising as draft for discussion for now.

[henryiii]

Is this required on Pyodide too? I seem to remember pytest not working and python -m pytest being required. On normal platforms, pytest is better than python -m pytest, since it does not add the current directory.

[joerick]

Is this required on Pyodide too?

I think it's a bit up in the air, but currently on macOS, python -m works better for pyodide also.

[henryiii]

I think this is better; in fact, I'd also be fine if not starting with python -m was an error, since it is required and it's a new feature.

[freakboy3742]

Interesting - I had a similar set of thoughts about this in the process of developing that particular feature. At the time, my thinking went the other direction: preferring the "short" form, and auto-stripping python -m from the start of a command line if it was provided. I opted not to do this to avoid Yet Another Weird iOS Behavior; however, I definitely recognise that there's a UX wart here.

The approach you've taken here makes more sense to me, and seems like it would ultimately be more robust. pytest ... will work; python -m pytest ... will work better; and ./run-test-suite.sh will fail at runtime in a way that should make some sense. So consider this a +1 from me to this idea.

Yes-anding this proposal - there are other predictable error conditions that we could catch in the case where the string doesn't start with python -m: anything where the first argument isn't a legal Python module name (i.e., (all(n.isidentifier() for n in name.split('.'))) could be immediately flagged as an error, with a similar "must provide a module" helper message.

[joerick]

Yes-anding this proposal - there are other predictable error conditions that we could catch in the case where the string doesn't start with python -m: anything where the first argument isn't a legal Python module name (i.e., (all(n.isidentifier() for n in name.split('.'))) could be immediately flagged as an error, with a similar "must provide a module" helper message.

Yeah, that's a nice idea. I'd be open to adding it if it's neat.

---

Another thought I had-

What should we do with the test-command if it includes other python flags, e.g. python -Pm MODULENAME, which was a pattern we recently discussed in #2358. I can see two options:

1. simply require python -m on iOS verbatim - it's not really being executed like that, it's more of a signifier than a real command.
2. Allow users to pass python -Pm or python -P -m, but don't bother with the other flags. -P might become a 'best practice' flag, so it would be nice to reuse config across platforms. -P shouldn't be relevant anyway, because test-sources has isolated the test cwd.
3. Try to parse the args more fully, and do something with that...

I think my preference is (1) - it's the most 'honest' version of this, since we'd be discarding any other interpreter flags anyway.

[freakboy3742]

I think my preference is (1) - it's the most 'honest' version of this, since we'd be discarding any other interpreter flags anyway.

I agree. Trying to parse the Python command line is going to be error prone at best. In the context of iOS, other Python command line arguments aren't going to be processed, so doing complex parsing to "pretend" that we are seems like asking for trouble.

[joerick]

Whoops! Apologies if you saw some extra commits here, they were meant for a different branch. Ready for review!

[freakboy3742]

This all makes sense to me - a couple of minor edge cases in testing flagged inline.

[freakboy3742]

It looks like a user-defined IPHONEOS_DEPLOYMENT_TARGET will no longer be considered as part of the expected wheel names?

Suggested change

	expected_wheels = utils.expected_wheels(
	"spam", "0.1.0", platform="ios", python_abi_tags=["cp313-cp313"]
	)
	expected_wheels = utils.expected_wheels(
	"spam",
	"0.1.0",
	platform="ios",
	python_abi_tags=["cp313-cp313"],
	iphoneos_deployment_target=os.getenv("IPHONEOS_DEPLOYMENT_TARGET", "13.0"),
	)

[joerick]

I've added the environment access to expected_wheels. BTW, if this is left unset, does a part of the toolchain set it, or is it determined by the iOS SDK used? I'm just wondering how good of a default "13.0" is.

[mhsmith]

In the bitarray package I found another example of a test command pattern which we should probably support, something of the form:

python -c "one_liner_which_runs_tests"

This could be passed to the testbed and executed using eval.

[freakboy3742]

python -c "one_liner_which_runs_tests"

This could be passed to the testbed and executed using eval.

Conceptually, sure; but it would need changes in the iOS testbed to support this. The testbed currently assumes a test can be executed as python -m.

[mhsmith]

In python/cpython#132870 I've generalized the Android testbed script to have -c and -m arguments, and I'll make cibuildwheel accept any of the following formats on Android:

• python -c
• python -m
• pytest – interpreted as python -m pytest

But the bitarray example also raises another question, which is independent of the form of the test command. iOS requires test-sources to be non-empty. But this package includes its test suite in the package itself, so empty is actually the correct value.

I can see a few possible solutions:

• Accept an empty value, and copy nothing. Packages which depend on some part of the project directory being available for the tests will fail, possibly in a confusing way.

• Use the same technique as xbuild-tools to distinguish between an explicit empty value (which copies nothing), and a default sentinel (which gives an error on mobile, and uses the project directory on desktop).

• Restore your original fallback approach – copying the project directory into the testbed app via an sdist. In the Android PR, in order to install build-system requirements for the build platform, I've added build as a direct dependency of cibuildwheel itself. So would now be possible to do this using [sys.executable, "-m", "build", "--sdist"].

I'm leaning towards the last one, because it maximizes the number of packages which will work without modification.

[freakboy3742]

I'm leaning towards the last one, because it maximizes the number of packages which will work without modification.

The downside to the --sdist approach is that it could be a little misleading as to whether things can/should be working. For most projects, it will copy a lot more than is needed for testing purposes (which could result in slow tests); and will end up with either a duplicate copy of the test code (if the project uses a src folder), or duplicated entries in the path (if the source tree contains a top level module). In many situations, it might work; but when it doesn't, it's going to fail in weird ways.

Of the options you gave, my preference would be (2) - that you have to specify something, but an empty list is an acceptable answer.

There is, of course, a fourth option that you didn't suggest: "Don't do that" :-) That is, encourage apps to have an actual test suite, rather than embedding test code in app code, by requiring a standalone test suite. That's possibly a little opinionated... but I don't think it's an entirely unreasonable position to hold.

[joerick]

Regarding python -c, I don't see a particular need to support that, even the bitarray example can be run just as well via python -m. But we can add it if you feel strongly.

Regarding the case of unnecessary test-sources. My instinct was to go with the sentinel approach, but we then have this question-

option value	iOS/Android	other platforms
unset	Invalid	run from project tree
./some-paths	run isolated with files	run isolated with files
(empty string)	run isolated without files	???

Probably the answer is to just "run isolated without files". But it means we now lack a way to override to the behaviour "run from project tree". It seems a shame to lose a feature for the sake of a rare edge-case.

I'd be open to 'do nothing'. The user could just workaround this with something silly, like test-sources = "README.md".

I'd also submit a fifth option, a documented special value for test-sources:

5. test-sources = "<no-files>" - means 'run isolated without files'. In that case, the table above would look like-

   option value	iOS/Android	other platforms
   unset	Invalid	run from project tree
   (empty string)	Invalid	run from project tree
   ./some-paths	run isolated with files	run isolated with files
   <no-files>	run isolated without files	run isolated without files

[mhsmith]

For most projects, it will copy a lot more than is needed for testing purposes (which could result in slow tests)

Agreed, so setting test-sources should still be encouraged, both to avoid this and the next issue.

and will end up with either a duplicate copy of the test code (if the project uses a src folder), or duplicated entries in the path (if the source tree contains a top level module).

Both of these things would also happen on desktop platforms when running the tests from the project directory, so at least we'd be consistent.

Regarding python -c, I don't see a particular need to support that, even the bitarray example can be run just as well via python -m.

In this case there is an alternative entry point that can be run with -m, but that might not always be the case. And it's not exactly the same, as it omits some debugging information which the package developer would probably want to see.

So I've added -c support to the Android PR, and we can add it later for iOS if it becomes necessary.

I'd be open to 'do nothing'. The user could just workaround this with something silly, like test-sources = "README.md".

OK, let's go with that for now – I'll add it to the documentation in the Android PR. If this turns out to be a common situation, then we can revisit this later.