Skip to content
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

Add a test-sources configuration option. #2062

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

freakboy3742
Copy link

When cibuildwheel runs a test suite for a wheel, it sets the working directory to a temporary folder. This ensures that the test suite is isolated from the project code, ensuring that the test suite is executed with only the code packaged in the wheel, and not code that is present in the project folder but not packaged.

Documentation for wheel testing encourages test suites to be run as test-command = "pytest {project}" (or similar) - that is, running pytest, but pointing pytest at the project directory for test discovery purposes. To make this pattern clear to the user, a dummy test_fail.py file is created in the test working directory; in the "naïve configuration case", this test will be discovered as the only test in the working directory, and will fail with a message about cibuildwheel's usage of a working directory.

This pattern guarantees that tests are run against the code in the packaged wheel, but ignores any test suite configuration in pyproject.toml (or other test configuration files), and can require additional code to support running tests via cibuildwheel, tox/nox, and local invocation from a single test codebase.

This leads to the use of patterns such as `test-command = "cd {project} && pytest" (used by Pillow, amongst others), which undoes the benefits of using an isolated test folder in the first place.

It is also a pattern that can't work for mobile environments (i.e., iOS and Android, as supported by PEP 730 and PEP 738). In a mobile app, the test must run on device (or simulated device), and any code to be executed must be bundled with the app. When running on the device with an installed wheel, {project} isn't a visible file system location; and shell commands (like cd {project}) can't be used.

To help prevent the cd {project} && pytest pattern, and to facilitate future PRs adding support for iOS and Android to cibuildwheel, this PR adds a new configuration item - test-sources. This setting is a list of files and folders, relative to {project}, that should be copied into the test working folder (including, if necessary, pyproject.toml or any other test configuration files).

The allows a simple test configuration to be specified as:

[tool.cibuildwheel]
test-command = "pytest"
test-sources = ["tests"]

A more complicated configuration, providing additional data plus a pytest.ini configuration file, would be something like:

[tool.cibuildwheel]
test-command = "pytest"
test-sources = ["pytest.ini", "path/to/other-data", "tests"]

If test-sources is not defined, the historical behavior (i.e., copying in the test_fail.py shim) is preserved.

Support for platform-specific variants (macos.test-sources), as well as environment variable configuration (CIBW_TEST_SOURCES, CIBW_TEST_SOURCES_MACOS etc) is also included.

@henryiii
Copy link
Contributor

I'll look at it in more detail later, but quick note: the "run in an isolated dir" feature isn't helpful for projects that follow a src structure (which many, but not all, compiled projects do follow, since there are a lot of issues running a compiled project that is not in a src structure). It's also not as important now with PYTHONSAFEPATH. So one option could be to allow this to be disabled, and maybe even do that by default in the future (3.0)?

@freakboy3742
Copy link
Author

I'll look at it in more detail later, but quick note: the "run in an isolated dir" feature isn't helpful for projects that follow a src structure (which many, but not all, compiled projects do follow, since there are a lot of issues running a compiled project that is not in a src structure). It's also not as important now with PYTHONSAFEPATH. So one option could be to allow this to be disabled, and maybe even do that by default in the future (3.0)?

True; that doesn't really change the situation for iOS/Android though, except in as much that "copy the entire project into the app" becomes a viable option. Even then, I'd argue being able to say "just copy these files/folders for the test" is a potentially useful feature, as there's lot of content in a project folder that you probably don't want to be copied into an app bundle, even for testing purposes.

@joerick
Copy link
Contributor

joerick commented Oct 30, 2024

Thanks for this and especially the great patch notes @freakboy3742 !

I'd be a bit reticent to change the testing behaviour across the board, especially moving test sources 'out-of-tree', because pytest does (sadly) add a bunch of stuff to PYTHONPATH that lots of users end up relying on. See #1534 for some notes on the pytest default imports. The behaviour that I think would be problematic is from tests.utils import helper - because the "tests" dir would have been moved out-of-tree already, it wouldn't be accessible at at import path. (This works because pytest does in fact add the target dir to the PYTHONPATH - the exact behaviour that the chdir thing was trying to solve!)

So I'd concur with @henryiii in that I'm very open to removing the 'tests run in a different dir' feature, as it doesn't actually protect us from the problem that it attempts to solve - in fact since 3.0 is coming up I'd be open to just removing that 'feature' regardless.

I'm very curious how that might change the feature design outlined here. One thing that I wonder is whether leave the test-command option as-is, and let us have a default value of . (the project dir) for test-sources, and then users scoping it down would comprise an optimisation, where fewer files have to be copied in the iOS/Android case. I guess then the test-sources command would only have effect on 'embedded' platforms (iOS/Android).

@henryiii
Copy link
Contributor

henryiii commented Oct 30, 2024

What I am thinking for 3.0 is:

  • test-sources = []: the default, just runs from the source directory. {project} is set to the source directory for compatibility.
  • test-sources = ["test", "pyproject.toml"]: Copy the listed files to a temporary directory and run the tests from there. This would work better than the current workaround since the pytest manipulations are actually fine in this case, as it's completely isolated from the original code. I'd either set {project} to the new directory, or make it an error, I think. This would be supported on all platforms.
  • test-sources = ["."] This could be the default on embedded platforms, which would copy everything over to the new location. It would be nice if it could be pre-copied, perhaps, to avoid any local caching to build directories?

@henryiii
Copy link
Contributor

henryiii commented Oct 30, 2024

Travis is failing with:

FAILED test/test_custom_repair_wheel.py::test - assert 'Build failed because a wheel named' in "\x1b[33mWarning\x1b[0m: cibuildwheel 3 will require Python 3.11+, please upgrade the Python version used to run cibui...ecutable doesn't exist in image 'quay.io/pypa/manylinux2014_x86_64:2024.10.21-1' to build 'pp38-manylinux_x86_64'.\n\n"

I bet this is due to the recent removal of pypy38 and pypy39 from the pre-installed versions in manylinux. I thought we were supposed to be using the manylinux-interpreters ensure tool from manylinux, though, @mayeut?

@mayeut
Copy link
Member

mayeut commented Oct 30, 2024

I thought we were supposed to be using the manylinux-interpreters ensure tool from manylinux

We are. Error handling/reporting should be updated to get a different error message when it fails (though I guess the log would be much clearer in a real execution than in the tests). The failure is probably related to a network related issue. Restarting the job now.

@freakboy3742
Copy link
Author

What I am thinking for 3.0 is:

  • test-sources = []: the default, just runs from the source directory. {project} is set to the source directory for compatibility.

To make sure I'm understanding this correctly - you're suggesting {project} doesn't change from the current behavior on main; but test_cwd is modified to be {project} (and the test_fail.py handling is removed)?

  • test-sources = ["test", "pyproject.toml"]: Copy the listed files to a temporary directory and run the tests from there. This would work better than the current workaround since the pytest manipulations are actually fine in this case, as it's completely isolated from the original code. I'd either set {project} to the new directory, or make it an error, I think. This would be supported on all platforms.

Again, if I'm understanding correctly, this is the same to what I've got in this PR, except for setting {project} during the test run. Am I correct?

  • test-sources = ["."] This could be the default on embedded platforms, which would copy everything over to the new location. It would be nice if it could be pre-copied, perhaps, to avoid any local caching to build directories?

It turns out that literally "copy everything" poses a problem, because that will frequently pick up virtual environments, tox/nox environments, GitHub configurations etc that are in the developer's project directory - and those usually contain symbolic links to locations outside the source tree (e.g., to the actual Python install). In the iOS case, these links aren't just inconvenient or excessive - they actually cause the iOS simulator conniptions because they link to filesystem locations that don't exist on the device.

In my WIP iOS branch, I've been using an sdist as a proxy for "copy all sources". My guess would be that we add a special case for the literal ["."] list as an "sdist copy" for this case.

@joerick
Copy link
Contributor

joerick commented Oct 31, 2024

What I am thinking for 3.0 is:

  • test-sources = []: the default, just runs from the source directory. {project} is set to the source directory for compatibility.
  • test-sources = ["test", "pyproject.toml"]: Copy the listed files to a temporary directory and run the tests from there. This would work better than the current workaround since the pytest manipulations are actually fine in this case, as it's completely isolated from the original code. I'd either set {project} to the new directory, or make it an error, I think. This would be supported on all platforms.

Ahh, that's clever. I like it. That way we actually get the protection that we were aiming for originally, but as an opt-in, with the current behaviour backward-compatible.

@henryiii
Copy link
Contributor

I wish there was a more elegant way to indicate the sdist-copy mechanism, which is really nice as a "copy all" replacement. But can't think of one.

@joerick
Copy link
Contributor

joerick commented Oct 31, 2024

I wish there was a more elegant way to indicate the sdist-copy mechanism, which is really nice as a "copy all" replacement. But can't think of one.

The sdist-copy is only really useful on the iOS/Android platforms, though? So perhaps we have a special case where test-sources: [] means do sdist-copy on that platform.

@freakboy3742
Copy link
Author

The sdist-copy is only really useful on the iOS/Android platforms, though?

iOS/Android requires a "copy sources" option. In that world, sdist-copy is a helpful default (and maps well to the "test_cwd is {project}" change described above), but if test-sources existed, sdist-copy didn't, that wouldn't be the end of the world; or, if sdist-copy existed as the iOS/Android default, but test-sources didn't, that would work fine as well.

As for other platforms - I'd argue sdist-copy isn't necessary, but it might be a nice safety mechanism. If a test suite does anything destructive to a filesystem (e.g., creating temp files in the test folder), using a copy of all sources guarantees there's no leakage between tests.

So perhaps we have a special case where test-sources: [] means do sdist-copy on that platform.

The complication would be differentiating between "Not defined" and "Empty", especially in the environment variable manifestation. I guess that's resolvable; it's then a question of whether [], ["."], or some other sentinel (a string "all"?) is a more obvious way to indicate "do a full copy".

@henryiii
Copy link
Contributor

henryiii commented Nov 1, 2024

[] would be an empty string environment variable - and I really don't like making a setting that depends on being empty vs. not being set. And TOML doesn't have a null value, so there wouldn't be a way to override it back to "no set" ether either.

SDist-copy isn't that useful outside of the embedded systems, though - it only protects you from local artifacts, not from importing local files. So I think having [] be sdist-copy and then only having two modes would be fine ([] for run in source or sdist-copy on embedded, and listing files and directories for a targeted copy). Then ["."] would have the same meaning as any other file or directory.

@joerick
Copy link
Contributor

joerick commented Nov 1, 2024

SDist-copy isn't that useful outside of the embedded systems, though - it only protects you from local artifacts, not from importing local files. So I think having [] be sdist-copy and then only having two modes would be fine ([] for run in source or sdist-copy on embedded, and listing files and directories for a targeted copy). Then ["."] would have the same meaning as any other file or directory.

Yes, this is what I meant, er, well, I think.

For clarity,

proposal A

test-sources

A list of files and directories containing tests, that are copied into an isolated temp folder from which the tests are run.

If left unset,

  • on iOS and Android, cibuildwheel makes an sdist of the source tree to copy files into the bundle.
  • on other platforms, tests are run directly from the project tree.

Note

If this option is left empty, and your project doesn't use a src directory structure, there's a risk of import confusion here - if your code does 'import mypackage.foo' and that's not bundled in the wheel, your tests could still pass because Python found the module locally instead. So it's advised to set the option to the minimum set of files required to run your tests.

Examples

CIBW_TEST_SOURCES: "test pyproject.toml"
test-sources = ["test", "pyproject.toml"]

Or, we could choose to be more explicit, using a parse_key_value_string - style option

Proposal B

test-sources

Controls what sources are available for the test step.

Default:

  • on iOS/Android, defaults to copy-sdist
  • on other platforms, defaults to project-dir

Options:

  • files: FILE_OR_DIR FILE_OR_DIR: Copies only the referenced files into an isolated directory to run the tests
  • project-dir: Runs the tests directly from the project dir. Not allowed on iOS/Android.
  • copy-sdist: Makes an sdist of the package and expands that into an isolated directory to run the tests

Note

If this option is set to project-dir or copy-sdist, and your project doesn't use a src directory structure, there's a risk of import confusion here - if your code does 'import mypackage.foo' and that's not bundled in the wheel, your tests could still pass because Python found the module locally instead. So it's advised to set the option to files: ... with the minimum set of files required to run your tests.

Examples

CIBW_TEST_SOURCES: "files: test pyproject.toml"
test-sources = { files: ["test", "pyproject.toml"] }

@joerick
Copy link
Contributor

joerick commented Nov 1, 2024

My preference is Proposal A, above, because there's no room for invalid configuration - you can't set project_dir on iOS/Android, and it avoids us having to make copy-sdist functionality on the other platforms (or making that a misconfiguration also) - as I can't see it being useful tbh.

Also, it just looks simpler, there's less to explain in the docs IMO.

@henryiii
Copy link
Contributor

henryiii commented Nov 1, 2024

I was thinking proposal A too. I don't think having access to SDist copy for other platforms is that important.

@freakboy3742
Copy link
Author

Looks like there's some sort of consensus around option A (which I agree looks like the best option - the benefits to copy-sdist for non-embedded projects are minimal).

In terms of implementation - AFAICT, this PR as written implements the "add a new test-sources setting" part. The remainder is to change the behavior of the "test-sources not defined" branch from "set the working directory to test_cwd" to "set the working directory to the project directory".

Would the preferred approach be to roll that changes into this PR, or to make a separate PR?

Also - given that this would be a backwards incompatible change, is there any additional paperwork required in the PR?

@freakboy3742
Copy link
Author

@henryiii @joerick I've taken the liberty of implementing "option A" (at least as I understand it) - that is:

  1. The default behavior of TEST_COMMAND is to run in the project directory.
  2. If the user specifies TEST_SOURCES, a temporary folder is created, files are copied into that temporary folder, and the temporary folder is used as the root when running tests.

As noted during the discussion about, this is a change in behavior; I'm not sure what procedures you have around documenting that for change notes.

The test_fail.py handling has been removed, as it's no longer applicable.

Any project that has a cd {project} as part of its test configuration (e.g., Pillow) can now remove that configuration. Having that extra statement won't break anything - it just becomes a no-op.

Any test suite that was relying on running with the working directory set to the temp folder will break - but I'm not sure I see how any project would be doing that in practice, as any relative file references in the test suite would have been failing.

@freakboy3742
Copy link
Author

Hrm - I wasn't getting those test failures locally on macOS... investigating...

@freakboy3742
Copy link
Author

I've now resolved the issue with CI failures - turns out I was somehow missing the exiting test of the "run pytest in the project directory". I've updated that test to validate a test suite will run in the project folder.

@henryiii henryiii added the Hold for future release This PR might be complete, but is scheduled to be merged in a future release. Don't merge yet. label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hold for future release This PR might be complete, but is scheduled to be merged in a future release. Don't merge yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants