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.

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
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 add UV_LOCKED: 1 here, so that we don't have to use --locked and --frozen anymore in the uv commands

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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]>
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