Skip to content
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

Use pyproject.toml to setup test discovery #443

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

theo-brown
Copy link
Contributor

I've added lines in pyproject.toml to configure pytest for automatically discovering tests, following #441.

This should:

  • Ignore torax/tests/scripts
  • Ignore torax/tests/test_data
  • Ignore any directory with lib in the name
  • Collect all files with a .py extension from directories called tests, at any level

This necessitated changing the names of the runtime_params test files in transport_models and sources to prevent filename conflicts.

I've modified the Github action to run the tests via auto-discovery.

I have deleted duplicate conftest.py files, and consolidated them into on file in the torax/ directory. I believe this will be shared between all the necessary tests, but I'm not completely sure.

@jcitrin
Copy link
Collaborator

jcitrin commented Oct 15, 2024

Thanks, this is very helpful!

Am testing it and realized that there are some Equinox issues, and also at least one unused broken test that we need to clean up. Possibly we'll want to add some more exclusions too. Will debug on our side and push to this PR.

@theo-brown
Copy link
Contributor Author

Yep, I think there were a few tests that were not previously being picked up by CI, but now are, that are failing.

I didn't spot the Equinox issues my side, but I didn't start digging into making all the tests work, so it could just be that I didn't look hard enough.

Definitely scope for more inclusions/exclusions - I wasn't certain which of the tests were purposefully vs accidentally being skipped in CI, so I decided to just push this to a PR so that others could work on it to.

@jcitrin
Copy link
Collaborator

jcitrin commented Oct 15, 2024

The Equinox issue was a non-issue, was just a non-updated venv on my side.

We'll take a look at further exclusions / cleanups. Thanks again!

torax/tests/sim.py \
torax/tests/state.py \
torax/transport_model/tests/qlknn_wrapper.py \
torax/transport_model/tests/transport_model.py \
Copy link
Member

Choose a reason for hiding this comment

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

Will torax/tests/sim_no_compile.py (referred to below) be handled correctly after this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for flagging. It won't but we're working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(relevant commit 029c24c)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants