pypa / cibuildwheel

🎡 Build Python wheels for all the platforms with minimal configuration.
https://cibuildwheel.pypa.io
Other
1.82k stars 231 forks source link

Multi-line test commands can spuriously pass #1732

Open itamarst opened 7 months ago

itamarst commented 7 months ago

Description

Let's say CIBW_TEST_COMMAND="false; true". On Unix this will be considered a success, even though one of the commands failed.

Here's a real example that tripped me up:

[tool.cibuildwheel]
test-command = """\
pip install -r {project}/requirements-dev.txt
pytest {project}/ -k 'not memory_usage'  # <-- this failed
pytest {project}/ -k 'memory_usage'      # <-- this passed
"""

Now, you could say that this is my responsibility and I should just add a set -e. But set -e I assume won't work on Windows, where the same commands will be run. So making sure failing commands cause tests to not spuriously pass is probably something cibuildwheel should do.

(I can do separate tests for Linux and Windows, but that's a workaround which leads to unnecessary duplication of scripts. This feels like a footgun that has probably hit other projects too. There's a reason most CI systems have set -e as the default.)

Build log

No response

CI config

No response

henryiii commented 7 months ago

You can use && or a list of strings and cibuildwheel will chain them with && for you. Works on all OSs.


[tool.cibuildwheel]
test-command = [
  "pip install -r {project}/requirements-dev.txt",
  "pytest {project}/ -k 'not memory_usage'",
  "pytest {project}/ -k 'memory_usage'",
]
itamarst commented 7 months ago

Thanks, good to know! The default still feels like a footgun though.

joerick commented 7 months ago

I agree, it is a shame, and it is a footgun. Unfortunately I don't think there is a way to get set -e behaviour on the default shell on Windows. I think we discussed it in the past, my feeling was that it was an even worse footgun if we only have the bad behaviour on Windows, since people are less likely to notice and fix the issue on one platform than across the whole build.

Options would be:

itamarst commented 7 months ago

Seems Powershell also has bad defaults :cry: With latest versions apparently this is how to fix them: https://github.com/PowerShell/PowerShell/issues/3415#issuecomment-1354457563