-
Notifications
You must be signed in to change notification settings - Fork 166
test: fix version_test
failing on old venv
#3134
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
Conversation
tests/version_test.py
Outdated
if version != pyproject_version: # pragma: no cover | ||
# NOTE: metadata from venv is outdated (https://github.com/narwhals-dev/narwhals/pull/3130#issuecomment-3291578373) | ||
version_comp = parse_version(version) | ||
assert version_comp < parse_version(pyproject_version) | ||
assert version_comp == parse_version(dist_version) | ||
assert version == dist_version |
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.
While I understand that locally this can be problematic (it happened to me multiple times), I am not sure that working around the test itself is the best solution here.
- In CI this should never happen, unless we are actually doing something wrong, and if that's the case, we could end up not noticing it.
- I would suggest to add a message in the current assert that explicitly suggestion why this happens and what to do, something along the lines of:
- assert nw.__version__ == pyproject_version
+ msg = "metadata is out of sync, either ignore this failure locally or re-install narwhals: `uv pip install -e .`"
+ assert nw.__version__ == pyproject_version, msg
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.
Thanks for the review @FBruzzesi
In CI this should never happen, unless we are actually doing something wrong, and if that's the case, we could end up not noticing it.
You make a good point! 😄
Would (2f22890) and (0500e3c) help alleviate this concern?
(provided I've RTFM correctly 😂)
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 am not sure that working around the test itself is the best solution here
I get that using pyproject.toml
provides a proxy value with a different static source (+1).
But as we don't use a lockfile, I don't think it quite fits this definition of assertion
... that always should evaluate to true at that point in code execution
We've both sadly experienced this not being an always - because of how it interacts with -e
😔
So the question for me was:
Is there anything else we can confidently assert, where a failure still indicates a regression?
Given that this is a very thin wrapper around importlib.metadata.version
our options are limited:
Lines 176 to 183 in 8bee263
def __getattr__(name: _t.Literal["__version__"]) -> str: # type: ignore[misc] | |
if name == "__version__": | |
global __version__ # noqa: PLW0603 | |
from importlib import metadata | |
__version__ = metadata.version(__name__) | |
return __version__ |
But we can use that to our advantage! To me, that implies the likely points for breakage are:
- A future refactor changes how we wrap
version
- We revert back to not using
version
In either case - we'll know that something is wrong because nw.__version__
no longer returns a string that is equal to importlib.metadata.version("narwhals")
.
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.
Thanks @dangotbanned 🙏🏼 nice trick for GHA 😉
What type of PR is this? (check all applicable)
Related issues
CompliantNamespace.is_native
#3130 (review)Checklist
If you have comments or can explain your changes, please do so below
(#3130 (comment))