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

Addressing overuse of fixtures #118

Merged
merged 24 commits into from
May 19, 2024
Merged

Addressing overuse of fixtures #118

merged 24 commits into from
May 19, 2024

Conversation

wiederm
Copy link
Member

@wiederm wiederm commented May 11, 2024

Description

We have accumulated technical debts in the configuration of some of our tests. This PR is a first attempt to clean up the overuse of slightly similar but different fixtures, for which the control flow is hidden in conftest.py, making it sometimes difficult to understand what is done in a particular test.

I opted for explicitly passing control parameters to each test instead of hiding these in fixtures. I have cleaned up some of the fixtures and simplified them where appropriate. We still use fixtures to set up datasets/databases/database containers. (Sidenote: In one of the next PRs, we need to address the inflationary use of the word dataset in the classes --- it is challenging to distinguish between some of the concepts without reading the documentation.)

Todos

  • simplify fixtures
  • clean up tests

Status

  • Ready to go

@wiederm wiederm linked an issue May 11, 2024 that may be closed by this pull request
@wiederm
Copy link
Member Author

wiederm commented May 12, 2024

@chrisiacovella , can you please look at the two tests still failing in the dataset test module? I can't figure out why these are not passing, I think it has to do with the caching.

@wiederm wiederm marked this pull request as ready for review May 12, 2024 06:58
@wiederm
Copy link
Member Author

wiederm commented May 13, 2024

I think that one is ready for review! @chrisiacovella , could you please take a look at the failing tests for the dataset, I can't figure out what is going wrong here.

@wiederm
Copy link
Member Author

wiederm commented May 18, 2024

Merging main into this branch led to some test failures that went undetected with the current test coverage in main. This PR extends the test coverage to all datasets and potentials that are implemented for most tests (there are exceptions) and clearly indicates which of them are used in a given test.

@wiederm wiederm merged commit 44a1701 into main May 19, 2024
4 of 5 checks passed
@wiederm wiederm deleted the ref-fixtures branch May 19, 2024 19:30
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.

Different datasets return tensors with different dimensions for the atomic_numbers property Dataset fixtures
1 participant