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

Fix internal tests invoked from zsh shell #1513

Merged
merged 3 commits into from
May 23, 2024

Conversation

haxoza
Copy link
Contributor

@haxoza haxoza commented May 22, 2024

Before this fix some of the internal hatch tests were failing when invoked from zsh shell. This change updates the test fixture to use the same shell detection mechanism as in the actual hatch code.

One of the test errors before the fix:

E       AssertionError: expected call not found.
E       Expected: append('/private/var/folders/q3/s01nm0vn50n3696zg_w711yr0000gn/T/tmpuovbeire/data/pythons/pypy3.9/pypy3.9-v6.3.15-macos_arm64/bin', shells=['sh'])
E         Actual: append('/private/var/folders/q3/s01nm0vn50n3696zg_w711yr0000gn/T/tmpuovbeire/data/pythons/pypy3.9/pypy3.9-v6.3.15-macos_arm64/bin', shells=['zsh'])
E
E       pytest introspection follows:
E
E       Kwargs:
E       assert {'shells': ['zsh']} == {'shells': ['sh']}
E
E         Differing items:
E         {'shells': ['zsh']} != {'shells': ['sh']}
E         Use -v to get more diff

Before this fix some of the internal hatch tests were failing when
invoked from zsh shell. This change updates the test fixture to use
the same shell detection mechanism as in the actual hatch code.
Copy link
Sponsor Collaborator

@ofek ofek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

from hatch.utils.platform import Platform


Shell: TypeAlias = tuple[str, str]
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we could please put the return type directly on the function without a dedicated type alias.

@@ -41,10 +43,7 @@ def install(app: Application, *, names: tuple[str, ...], private: bool, update:
from hatch.python.distributions import ORDERED_DISTRIBUTIONS
from hatch.python.resolve import get_distribution

shells = []
if not private and not app.platform.windows:
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to revert this part because we want this conditional to prevent the shell detection logic for performance reasons.

Copy link
Sponsor Collaborator

@ofek ofek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again!

@ofek ofek merged commit 0e6c93a into pypa:master May 23, 2024
46 checks passed
@haxoza
Copy link
Contributor Author

haxoza commented May 23, 2024

Thanks for merging it and your feedback, I appreciate! I'm sorry I didn't improve the PR myself but I was on my way back home from PyCon.

@ofek
Copy link
Sponsor Collaborator

ofek commented May 23, 2024

No worries, it was a fun conference!

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.

None yet

2 participants