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

👷‍♂️ Consistently use sys.executable to run subprocesses, needed by OpenSUSE #408

Merged

Conversation

theMarix
Copy link
Contributor

In some scenarios, like running pytest outside of an active virtualenv,
it could happen that "coverage" would refer to a different Python
interpreter than the one running the tests. Admittedly that does not
happen in standard development scenarios where you use a virtualenv. But
it easily happens when packaging for Linux distributions that support
multiple versions of Python and it can also happen when running pytest
from a virtualenv without activating it. The latter is something that's
convenient when testing versus multiple Python versions without using
tox.

Explicitly invoking coverage as a module on sys.executable ensures that
the same binary, not process, that's also running the tests is used.
This was in fact already done in some tests but not consistently across
all of them.

@github-actions
Copy link

📝 Docs preview for commit 04a0e70 at: https://62b5fdf9cc5bd304bad2e920--typertiangolo.netlify.app

@theMarix theMarix force-pushed the run-subprocesses-with-correct-python branch from 04a0e70 to 7fce85d Compare June 24, 2022 18:11
@codecov
Copy link

codecov bot commented Jun 24, 2022

Codecov Report

Base: 96.24% // Head: 100.00% // Increases project coverage by +3.75% 🎉

Coverage data is based on head (04a0e70) compared to base (1e43c6b).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 04a0e70 differs from pull request most recent head 501e25e. Consider uploading reports for the commit 501e25e to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #408      +/-   ##
===========================================
+ Coverage   96.24%   100.00%   +3.75%     
===========================================
  Files         280       252      -28     
  Lines        5942      5370     -572     
===========================================
- Hits         5719      5370     -349     
+ Misses        223         0     -223     
Impacted Files Coverage Δ
tests/test_completion/test_completion.py 100.00% <ø> (ø)
tests/test_compat/test_option_get_help.py 100.00% <100.00%> (ø)
tests/test_completion/test_completion_complete.py 100.00% <100.00%> (ø)
...est_completion/test_completion_complete_no_help.py 100.00% <100.00%> (ø)
tests/test_completion/test_completion_install.py 100.00% <100.00%> (ø)
tests/test_completion/test_completion_show.py 100.00% <100.00%> (ø)
tests/test_others.py 100.00% <100.00%> (ø)
tests/test_prog_name.py 100.00% <100.00%> (ø)
...al/test_arguments/test_default/test_tutorial001.py 100.00% <100.00%> (ø)
...al/test_arguments/test_default/test_tutorial002.py 100.00% <100.00%> (ø)
... and 210 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@theMarix theMarix force-pushed the run-subprocesses-with-correct-python branch from 7fce85d to b2ed091 Compare July 18, 2022 10:12
@github-actions
Copy link

github-actions bot commented Nov 5, 2022

📝 Docs preview for commit 9bc91d5 at: https://6366753ee3a47378ef50daa8--typertiangolo.netlify.app

@github-actions
Copy link

github-actions bot commented Nov 5, 2022

📝 Docs preview for commit fde13f7 at: https://636675ef52cfab79cb9f3258--typertiangolo.netlify.app

@theMarix theMarix force-pushed the run-subprocesses-with-correct-python branch from fde13f7 to 53eaf7e Compare November 6, 2022 19:06
@github-actions
Copy link

github-actions bot commented Nov 6, 2022

📝 Docs preview for commit 53eaf7e at: https://636805da6d69cd0402127c6d--typertiangolo.netlify.app

In some scenarios, like running pytest outside of an active virtualenv,
it could happen that "coverage" would refer to a different Python
interpreter than the one running the tests. Admittedly that does not
happen in standard development scenarios where you use a virtualenv. But
it easily happens when packaging for Linux distributions that support
multiple versions of Python and it can also happen when running pytest
from a virtualenv without activating it. The latter is something that's
convenient when testing versus multiple Python versions without using
tox.

Explicitly invoking coverage as a module on sys.executable ensures that
the same binary, not process, that's also running the tests is used.
This was in fact already done in some tests but not consistently across
all of them.
@theMarix theMarix force-pushed the run-subprocesses-with-correct-python branch from 53eaf7e to 1f64f55 Compare November 6, 2022 19:18
@github-actions
Copy link

github-actions bot commented Nov 6, 2022

📝 Docs preview for commit 1f64f55 at: https://636808c83d1e860a554c222b--typertiangolo.netlify.app

@theMarix
Copy link
Contributor Author

theMarix commented Nov 6, 2022

Curiously, the build failures triggered by this PR are exactly the ones that #407 fixes. Not sure how best to proceed here. Ideally we can agree to merge #407 and than I'd rebase this onto master and everything will just work™.

@github-actions
Copy link

github-actions bot commented Nov 7, 2022

📝 Docs preview for commit 501e25e at: https://636946baf62f600f21622fd6--typertiangolo.netlify.app

@tiangolo tiangolo changed the title Consistently use sys.executable to run subprocesses 👷‍♂️ Consistently use sys.executable to run subprocesses, needed by OpenSUSE Nov 7, 2022
@tiangolo
Copy link
Member

tiangolo commented Nov 7, 2022

Cool, thanks! Let's do it! 🚀

@tiangolo tiangolo merged commit 416c612 into fastapi:master Nov 7, 2022
@theMarix theMarix deleted the run-subprocesses-with-correct-python branch November 7, 2022 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants