-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: match os.PathLike too #109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a test, with the real subprocess usage by calling the example_script.py
- see test_basic_process
for an example. It will help to ensure that the mock behaves exactly the same as the subprocess. I think it will require also adding a check for the python version to behave appropriately on python < 3.8.
Python 3.6/3.7 should happen to work too, as long as it’s not Windows. I’ll try to add this soon. |
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
2a9f934
to
971ae09
Compare
Codecov Report
@@ Coverage Diff @@
## master #109 +/- ##
=============================
=============================
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Henry Schreiner <[email protected]>
943db11
to
490d02a
Compare
tests/test_subprocess.py
Outdated
@@ -13,6 +14,20 @@ | |||
|
|||
PYTHON = sys.executable | |||
|
|||
win_path_skip = pytest.mark.skipif( | |||
sys.platform.startswith("win") and sys.version_info < (3, 8), | |||
reason="Path in subprocess not supported before 3.8 on Windows", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of skipping it on Windows for 3.7, I'd rather prefer to behave similarly to the real subprocess on this platform. I see it raises TypeError
there, so I think this plugin should recreate this behavior to be reliable.
2de0b7c
to
2bf339b
Compare
Signed-off-by: Henry Schreiner <[email protected]>
2bf339b
to
bb9bab7
Compare
) | ||
out, err = process.communicate() | ||
if ( | ||
not fake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've changed the tests to verify exactly what happens, rather than skipping. I think what you are asking is you'd like the not fake
condition above to be removed? That's a bit of a follow up (a feature that touches some of the same code, but is an independent improvement). This improvement specifically makes Path("x")
match "x"
, while before they only matched if you called the subprocess with the exact same object as the registration set. Having Path("x")
throw the same error as the real subprocess on Win < 3.8 seems like another good addition. I can do that here or in a followup. (PS: going on vacation tomorrow, so might not be doing too much this week).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant. There is no rush with this (unless you do need it fast?), so we can wait for you to come back from vacation or we can merge it as is now and work on this TypeError
later (I could also work on it if you prefer). Let me know what you prefer, I'm OK with both.
And have a nice vacation :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor preference to merge this, then work on TypeError, since it's a distinct feature (that happens to modify this test).
I'll see if I can work on it in the next couple of hours, otherwise it will likely wait or be something you work on. :)
Unless you are in a rush to release something, I'd like to test this out with scikit-build-core pulling the plugin from git once everything is in to see how the recents fixes works in a project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not in a rush at all, I can wait for a notification from your side that all is OK and I should release a new version.
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [fastapi](https://togithub.com/tiangolo/fastapi) | `==0.89.0` -> `==0.89.1` | [![age](https://badges.renovateapi.com/packages/pypi/fastapi/0.89.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/fastapi/0.89.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/fastapi/0.89.1/compatibility-slim/0.89.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/fastapi/0.89.1/confidence-slim/0.89.0)](https://docs.renovatebot.com/merge-confidence/) | | [pyright](https://togithub.com/RobertCraigie/pyright-python) | `==1.1.287` -> `==1.1.292` | [![age](https://badges.renovateapi.com/packages/pypi/pyright/1.1.292/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/pyright/1.1.292/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/pyright/1.1.292/compatibility-slim/1.1.287)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/pyright/1.1.292/confidence-slim/1.1.287)](https://docs.renovatebot.com/merge-confidence/) | | [pytest](https://docs.pytest.org/en/latest/) ([source](https://togithub.com/pytest-dev/pytest), [changelog](https://docs.pytest.org/en/stable/changelog.html)) | `==7.2.0` -> `==7.2.1` | [![age](https://badges.renovateapi.com/packages/pypi/pytest/7.2.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/pytest/7.2.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/pytest/7.2.1/compatibility-slim/7.2.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/pytest/7.2.1/confidence-slim/7.2.0)](https://docs.renovatebot.com/merge-confidence/) | | [pytest-subprocess](https://togithub.com/aklajnert/pytest-subprocess) | `==1.4.2` -> `==1.5.0` | [![age](https://badges.renovateapi.com/packages/pypi/pytest-subprocess/1.5.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/pytest-subprocess/1.5.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/pytest-subprocess/1.5.0/compatibility-slim/1.4.2)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/pytest-subprocess/1.5.0/confidence-slim/1.4.2)](https://docs.renovatebot.com/merge-confidence/) | | [slotscheck](https://togithub.com/ariebovenberg/slotscheck) | `==0.16.3` -> `==0.16.4` | [![age](https://badges.renovateapi.com/packages/pypi/slotscheck/0.16.4/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/slotscheck/0.16.4/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/slotscheck/0.16.4/compatibility-slim/0.16.3)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/slotscheck/0.16.4/confidence-slim/0.16.3)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>tiangolo/fastapi</summary> ### [`v0.89.1`](https://togithub.com/tiangolo/fastapi/releases/tag/0.89.1) [Compare Source](https://togithub.com/tiangolo/fastapi/compare/0.89.0...0.89.1) ##### Fixes - 🐛 Ignore Response classes on return annotation. PR [#​5855](https://togithub.com/tiangolo/fastapi/pull/5855) by [@​Kludex](https://togithub.com/Kludex). See the new docs in the PR below. ##### Docs - 📝 Update docs and examples for Response Model with Return Type Annotations, and update runtime error. PR [#​5873](https://togithub.com/tiangolo/fastapi/pull/5873) by [@​tiangolo](https://togithub.com/tiangolo). New docs at [Response Model - Return Type: Other Return Type Annotations](https://fastapi.tiangolo.com/tutorial/response-model/#other-return-type-annotations). - 📝 Add External Link: FastAPI lambda container: serverless simplified. PR [#​5784](https://togithub.com/tiangolo/fastapi/pull/5784) by [@​rafrasenberg](https://togithub.com/rafrasenberg). ##### Translations - 🌐 Add Turkish translation for `docs/tr/docs/tutorial/first_steps.md`. PR [#​5691](https://togithub.com/tiangolo/fastapi/pull/5691) by [@​Kadermiyanyedi](https://togithub.com/Kadermiyanyedi). </details> <details> <summary>RobertCraigie/pyright-python</summary> ### [`v1.1.292`](https://togithub.com/RobertCraigie/pyright-python/compare/v1.1.291...v1.1.292) [Compare Source](https://togithub.com/RobertCraigie/pyright-python/compare/v1.1.291...v1.1.292) ### [`v1.1.291`](https://togithub.com/RobertCraigie/pyright-python/compare/v1.1.290...v1.1.291) [Compare Source](https://togithub.com/RobertCraigie/pyright-python/compare/v1.1.290...v1.1.291) ### [`v1.1.290`](https://togithub.com/RobertCraigie/pyright-python/compare/v1.1.289...v1.1.290) [Compare Source](https://togithub.com/RobertCraigie/pyright-python/compare/v1.1.289...v1.1.290) ### [`v1.1.289`](https://togithub.com/RobertCraigie/pyright-python/compare/v1.1.288...v1.1.289) [Compare Source](https://togithub.com/RobertCraigie/pyright-python/compare/v1.1.288...v1.1.289) ### [`v1.1.288`](https://togithub.com/RobertCraigie/pyright-python/compare/v1.1.287...v1.1.288) [Compare Source](https://togithub.com/RobertCraigie/pyright-python/compare/v1.1.287...v1.1.288) </details> <details> <summary>pytest-dev/pytest</summary> ### [`v7.2.1`](https://togithub.com/pytest-dev/pytest/releases/tag/7.2.1) [Compare Source](https://togithub.com/pytest-dev/pytest/compare/7.2.0...7.2.1) # pytest 7.2.1 (2023-01-13) ## Bug Fixes - [#​10452](https://togithub.com/pytest-dev/pytest/issues/10452): Fix 'importlib.abc.TraversableResources' deprecation warning in Python 3.12. - [#​10457](https://togithub.com/pytest-dev/pytest/issues/10457): If a test is skipped from inside a fixture, the test summary now shows the test location instead of the fixture location. - [#​10506](https://togithub.com/pytest-dev/pytest/issues/10506): Fix bug where sometimes pytest would use the file system root directory as `rootdir <rootdir>`{.interpreted-text role="ref"} on Windows. - [#​10607](https://togithub.com/pytest-dev/pytest/issues/10607): Fix a race condition when creating junitxml reports, which could occur when multiple instances of pytest execute in parallel. - [#​10641](https://togithub.com/pytest-dev/pytest/issues/10641): Fix a race condition when creating or updating the stepwise plugin's cache, which could occur when multiple xdist worker nodes try to simultaneously update the stepwise plugin's cache. </details> <details> <summary>aklajnert/pytest-subprocess</summary> ### [`v1.5.0`](https://togithub.com/aklajnert/pytest-subprocess/blob/HEAD/HISTORY.rst#​150-2023-01-28) [Compare Source](https://togithub.com/aklajnert/pytest-subprocess/compare/1.4.2...1.5.0) Features * `#​109 <https://github.com/aklajnert/pytest-subprocess/pull/109>`_: Match also `os.PathLike`. * `#​105 <https://github.com/aklajnert/pytest-subprocess/pull/105>`_: Add program matcher. Other changes - `#​110 <https://github.com/aklajnert/pytest-subprocess/pull/110>`\_: Produce TypeError on Win Py<3.8 for Path args. </details> <details> <summary>ariebovenberg/slotscheck</summary> ### [`v0.16.4`](https://togithub.com/ariebovenberg/slotscheck/blob/HEAD/CHANGELOG.rst#​0164-2023-01-13) [Compare Source](https://togithub.com/ariebovenberg/slotscheck/compare/v0.16.3...v0.16.4) - Remove `tomli` dependency on Python 3.11+ </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/RobertCraigie/prisma-client-py). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC45Ny4xIiwidXBkYXRlZEluVmVyIjoiMzQuMTE5LjAifQ==--> --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Robert Craigie <[email protected]>
Starting in Python 3.8 (and older versions on UNIX), subprocess accepts
os.PathLike
arguments. This adds support for that infp.register
too.Closes #101.