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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think pytest will be installed though...

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.

Hmm I haven't seen this one before 🤔. What problem does this solve?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is included automatically by https://github.com/scientific-python/cookie IIRC

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 guessing that scientific-python/cookie#405 is the reason for it then, which links to https://force-color.org/:

By adopting this standard, users who want colored text output, even in situations like piping or CI environments can simply export FORCE_COLOR=1 to their environment. This will automatically enable color by default in all software that follows the standard.

And here's the explanation for why 3 is used, and not 1:

One library that I know of (one I wrote years ago, to be fair) - plumbum.colors - takes levels for FORCE_COLOR, with 1 being 8-color, 2 being 16-color, 3 being 256 color, and 4 being true color, based on the four ways to do ANSI codes. I've always hoped other libraries would pick up on that, so I've always set it to 3. As far as I know, most other libraries just care if it's either defend or set to any value.


I mean, it's pretty interesting. Maybe it's just me (e.g. because I'm colorblind, or because it's almost 4 a.m. here), but I still don't see what problem this solves 😛.

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 actually don't know either. I've always included it, so don't know what the output looks like when removed...

check_oldest:
name: Check Oldest Dependencies
runs-on: ${{ matrix.runs-on }}
needs: [format]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might slow down the CI runs a bit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little. Depends how fast the pre-commit check is.

@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.

@nstarman nstarman force-pushed the tests branch 5 times, most recently from b84a599 to a569ad9 Compare June 9, 2025 03:45
Co-authored-by: Joren Hammudoglu <[email protected]>
Signed-off-by: Nathaniel Starkman <[email protected]>
@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
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