Skip to content

✅ 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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

✅ test: add tests & CI #18

wants to merge 2 commits into from

Conversation

nstarman
Copy link
Collaborator

@nstarman nstarman commented Jun 7, 2025

Requires #17

@nstarman nstarman added this to the v2021-12-0.0 milestone Jun 7, 2025
@jorenham jorenham added ✅ tests Add, update, or pass tests. 👷 CI Add or update CI build system. labels Jun 7, 2025
Comment on lines 73 to 64

- name: Install the project
run: uv sync --group test
Copy link
Collaborator

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.

Suggested change
- name: Install the project
run: uv sync --group test

Copy link
Collaborator Author

@nstarman nstarman Jun 9, 2025

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.

@nstarman nstarman force-pushed the tests branch 3 times, most recently from 542d303 to be6ff4e Compare June 9, 2025 02:52
@nstarman
Copy link
Collaborator Author

nstarman commented Jun 9, 2025

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 nstarman force-pushed the tests branch 2 times, most recently from 6c9a852 to 20159bb Compare June 9, 2025 03:02
Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

@jorenham jorenham Jun 9, 2025

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

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.

@nstarman nstarman force-pushed the tests branch 6 times, most recently from a569ad9 to 4b77c67 Compare June 9, 2025 03:49
@nstarman nstarman marked this pull request as ready for review June 9, 2025 03:53
@nstarman nstarman requested review from lucascolley and jorenham June 9, 2025 03:53
Copy link
Collaborator

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"
Copy link
Collaborator

@jorenham jorenham Jun 13, 2025

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

Suggested change
"mypy>=1.13.0"
"mypy>=1.16.0"

Comment on lines 47 to 50
# TODO: check when pypy3.11 is available
# include:
# - python-version: pypy-3.11
# runs-on: ubuntu-latest
Copy link
Collaborator

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]
Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

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.

Copy link
Member

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

Copy link
Collaborator Author

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?

Copy link
Member

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

Copy link
Collaborator

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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👷 CI Add or update CI build system. ✅ tests Add, update, or pass tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants