-
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.
@@ -51,6 +51,9 @@ | |||
"pytest-github-actions-annotate-failures>=0.3.0", | |||
"sybil>=8.0.0", | |||
] | |||
mypy = [ | |||
"mypy>=1.13.0" |
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 can matter quite a bit
"mypy>=1.13.0" | |
"mypy>=1.16.0" |
.github/workflows/ci.yml
Outdated
# TODO: check when pypy3.11 is available | ||
# include: | ||
# - python-version: pypy-3.11 | ||
# runs-on: ubuntu-latest |
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.
NumPy recently stopped building wheels for pypy, mostly because there's not enough interest in it anymore, and I believe that the maintainers are not planning on developing it further.
But if we can enable this for "free", then I don't see why not. But if it turns out that supporting pypy comes at a cost (e.g. CI slowdown because of missing wheels), then I don't think that we should. So in MoSCoW-speak, I'd categorize it as a "could-have", and I personally wouldn't use a # TODO
for that.
src docs tests | ||
|
||
- name: Upload coverage report | ||
uses: codecov/[email protected] |
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.
Do you think we should we go major-only (@v5
in this case), or use this patch-level version spec everywhere? Personally I'm +0 on this.
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.
Defer to Dependabot, with grouped updates and filtering on inclusions in releases?
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.
It might become mildy annoying to have to merge many dependabot PR though. But that's just speculation.
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.
you could enable automerge for dependabot
Also, we should be using full commit hashes (check out https://scientific-python.org/specs/spec-0008/).
FWIW, in array-api-extra I have set up renovate to send a monthly PR to update actions, it works well: data-apis/array-api-extra#293
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. @lucascolley do you want to commit directly this PR?
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 I have commit access
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 good idea @lucascolley; following SPEC 8 makes a lot of sense sense here.
Co-authored-by: Joren Hammudoglu <[email protected]> Signed-off-by: Nathaniel Starkman <[email protected]>
Requires #17