✅ test: add tests & CI #18
Conversation
jorenham
added
✅ tests
.github/workflows/ci.yml
Outdated
|
|
||
| - name: Install the project | ||
| run: uv sync --group test |
There was a problem hiding this comment.
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.
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.
nstarman
force-pushed
the
tests
branch
3 times, most recently
from
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]>
nstarman
force-pushed
the
tests
branch
2 times, most recently
from
6c9a852 to
20159bb
Compare
tests/test_namespace.py
Outdated
There was a problem hiding this comment.
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.
We could also rm this, and leave it to the integration tests in a follow-up, AFAIK.
There was a problem hiding this comment.
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.
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.
SGTM! Want to open a PR for that?
There was a problem hiding this comment.
Yea, sure. But it probably won't be today or tommorrow.
nstarman
force-pushed
the
tests
branch
6 times, most recently
from
a569ad9 to
4b77c67
Compare
tests/test_namespace.py
Outdated
There was a problem hiding this comment.
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.
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.
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.
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.
Defer to Dependabot, with grouped updates and filtering on inclusions in releases?
There was a problem hiding this comment.
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.
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.
SGTM. @lucascolley do you want to commit directly this PR?
There was a problem hiding this comment.
I don't think I have commit access
There was a problem hiding this comment.
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