-
Notifications
You must be signed in to change notification settings - Fork 4
✅ test: add tests & CI #18
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
base: main
Are you sure you want to change the base?
Conversation
.github/workflows/ci.yml
Outdated
|
||
- name: Install the project | ||
run: uv sync --group test |
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 don't think that --group
plays well with caching, because we use different groups in different jobs with the same cache key. Uv is already insanely fast, so I don't expect that using different cache keys will help much in practice. So I suppose we could just install everything, and skip this step altogether.
- name: Install the project | |
run: uv sync --group test |
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 see.
The uv
docs are pretty self-contradictory in terms of advice for best practices.
Following https://docs.astral.sh/uv/guides/integration/github/#multiple-python-versions, what I have now is good.
Can we run down the minutiae in a followup PR? This setup follows recommendations and works, but may not be 100% what we want.
542d303
to
be6ff4e
Compare
I'd like to figure out how to get the CI going to test the suggested changes to the CI! (I know the current setup works) |
Signed-off-by: Nathaniel Starkman <[email protected]>
6c9a852
to
20159bb
Compare
tests/test_namespace.py
Outdated
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 don't love this test. xpt.HasArrayNamespace
is final and not runtime-checkable. This is a bit of a hack.
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.
We could also rm this, and leave it to the integration tests in a follow-up, AFAIK.
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 do want to refactor this, but for the moment working tests are better than None 🤷 .
I think the way we want to refactor this might be to something similar to https://github.com/python/mypy/tree/master/test-data/unit
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 think the way we want to refactor this might be to something similar to python/mypy@
master
/test-data/unit
That looks horrible haha. I'd prefer a more IDE-friendly approach like https://github.com/numpy/numtype/tree/main/src/numpy-stubs/%40test , possibly with automatic test-generation like numtype also does.
In https://github.com/scipy/scipy-stubs/tree/master/tests I explained the mechanisms of this testing method a bit better.
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.
SGTM! Want to open a PR for that?
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.
Yea, sure. But it probably won't be today or tommorrow.
a569ad9
to
4b77c67
Compare
tests/test_namespace.py
Outdated
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.
We could also rm this, and leave it to the integration tests in a follow-up, AFAIK.
env: | ||
# Many color libraries just need this to be set to any value, but at least | ||
# one distinguishes color depth, where "3" -> "256-bit color". | ||
FORCE_COLOR: 3 |
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.
we could add UV_LOCKED: 1
here, so that we don't have to use --locked
and --frozen
anymore in the uv
commands
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.
Does that work when we have the --resolution lowest-direct
?
Since the CI isn't on (hmm...) it's hard to tell.
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 sure if --resolution lowest-direct
is the best way to go here actually. Maybe I'm wrong here, but I thought that SPEC 0 applied to the latest patch release of a minor 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.
We still want to test the oldest versions of all supported dependencies.
Co-authored-by: Joren Hammudoglu <[email protected]> Signed-off-by: Nathaniel Starkman <[email protected]>
Requires #17