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

Fixing known test issues and small test refactoring. #88

Merged
merged 34 commits into from
Apr 27, 2023
Merged

Conversation

mattkwiecien
Copy link
Contributor

@mattkwiecien mattkwiecien commented Apr 25, 2023

Summary

Our tests don't consistently pass, which can be misleading for new contributors who find their new PR may lead to a failed build.

This was caused by 2 main issues, not completely tearing down the tests (e.g. cleanup) and a runtime error thrown by NaMaster that bubbles up the stack into our code.

With these changes I verified all tests pass (individually or all together) and all temporary files get cleaned up

NaMaster issue

  • We call write_to on namaster workspace, covariances and this writes to a file. This can randomly throw a runtime exception "Unable to write to file"
  • This error occurs randomly, about 20% of the time
  • I tried locking and touching the file to make sure it exists, running synchronously, as well as writing to a separate temp directory. Nothing really changes the outcome.
  • To solve this, I added pytest-retry which allows us to re-run tests that fail, I allow the tests affected by this bug to re-try 5 times. If we succeed once, we know it was the NaMaster issue and we pass the test.

Test changes

  • I updated the tests to use setup_module() and teardown_module(). These run once at the beginning of each test file. This allows us to have more controlled reading/writing of files without constantly needing to check if files exist or deleting files that should still exist.
  • Following typical naming conventions, I renamed any global scope variables in tests to be all caps
  • I updated some of the mocks we use to be fixtures when possible for a more consistent testing approach
  • All of the tests now write to tests/tmp/directory. Before they were writing to separate directories.
    • I originally had all tests writing to their own tmp/ subdirectory. This works but because of the code structure we would need 1 configuration file per test.
    • Instead of doing this now I left it as 1 directory, and if we ever run tests in parallel we can change this then.
  • I moved all the configuration files for the tests to the same directory
  • I removed any code that ran automatically at the global scope in the tests. This stops files from being arbitrarily written without actually running the tests.

Code Changes

  • I added a clobber argument to get_covariance_block to allow a user to recalculate the covariance if they want to. Currently the only way to do that is manual or re-instantiate the class. This simplifies the test as well as giving more functionality to the user.

Build changes

  • pip 23.1 officially removed support for --no-bin (see comments), this was causing mpich tests to fail. I updated the build script to use the new option --no-cache-dir

@mattkwiecien
Copy link
Contributor Author

Note for myself

DEPRECATION: --no-binary currently disables reading from the cache of locally built wheels. In the future --no-binary will not influence the wheel cache. pip 23.1 will enforce this behaviour change. A possible replacement is to use the --no-cache-dir option. You can use the flag --use-feature=no-binary-enable-wheel-cache to test the upcoming behaviour. Discussion can be found at pypa/pip#11453

Copy link
Contributor

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

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

Great work! All good. Just a few minor comments/questions.

tests/test_covariance_fourier_base.py Show resolved Hide resolved
tests/test_covariance_fourier_gaussian_nmt.py Show resolved Hide resolved
tests/test_covariance_fourier_ssc.py Show resolved Hide resolved
tests/test_covariance_real_base.py Show resolved Hide resolved
tjpcov/covariance_fourier_gaussian_nmt.py Show resolved Hide resolved
Copy link
Contributor

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

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

LGTM

@mattkwiecien mattkwiecien merged commit 1a06804 into master Apr 27, 2023
@mattkwiecien mattkwiecien deleted the ci-fixes branch April 27, 2023 14:42
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.

2 participants