Skip to content

Conversation

svlandeg
Copy link
Member

@svlandeg svlandeg commented Jul 24, 2024

When we generalized the test suite to also run on Windows and Mac, we introduced a fixture needs_linux to only run two specific tests on Bash/Linux. However, these specific tests may fail on a Linux system with a different shell. So, this PR adds the fixture to needs_bash as well. Only keeping needs_bash is not sufficient as MacOS tests will fail on the CI.

Thanks to @rokbot for pointing this out in #866.

In follow-up work, ideally we should extend these tests to make them more applicable across OS and shell versions, but I want to get this quick fix in first to avoid having the test suite fail on non-bash Linux systems.

@svlandeg svlandeg changed the title Adjust test fixture to needs_bash instead of needs_linux ✅ Adjust test fixture to needs_bash instead of needs_linux Jul 24, 2024
@svlandeg svlandeg added bug Something isn't working repo / tests Involving the CI / test suite p2 labels Jul 24, 2024
@svlandeg svlandeg marked this pull request as draft July 24, 2024 11:24
@svlandeg svlandeg changed the title ✅ Adjust test fixture to needs_bash instead of needs_linux ✅ Add needs_bash test fixture Jul 24, 2024
@svlandeg svlandeg marked this pull request as ready for review July 24, 2024 12:00
@tiangolo tiangolo changed the title ✅ Add needs_bash test fixture ✅ Add needs_bash test fixture Aug 24, 2024
@tiangolo tiangolo added internal and removed bug Something isn't working labels Aug 24, 2024
@tiangolo
Copy link
Member

Great, thank you @svlandeg! 🚀

@tiangolo tiangolo merged commit a5b7557 into fastapi:master Aug 24, 2024
25 checks passed
@svlandeg svlandeg deleted the feat/needs_bash branch August 26, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal p2 repo / tests Involving the CI / test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants