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

Add tests/clean fix_level_coord() #90

Merged
merged 8 commits into from
Aug 29, 2024
Merged

Add tests/clean fix_level_coord() #90

merged 8 commits into from
Aug 29, 2024

Conversation

truth-quark
Copy link
Collaborator

Closes #53. Note, this PR is merges to main.

This PR:

  • adds unit tests for fix_level_coord()
  • moves the function down to call order (i.e. below process())
  • Cleans the code slightly

These tests use iris coordinate objects to avoid mocks cube attributes. Based on this experiment, this could be a reasonable approach for other tests requiring iris coordinates.

Any comments welcome!

@truth-quark truth-quark self-assigned this Aug 23, 2024
Copy link
Collaborator

@blimlim blimlim left a comment

Choose a reason for hiding this comment

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

Looks good! The only change is the fix_level_coord docstring, while the other two are questions about the clarity for the tests and so aren't completely necessary.

umpost/um2netcdf.py Outdated Show resolved Hide resolved
test/test_um2netcdf.py Outdated Show resolved Hide resolved
test/test_um2netcdf.py Outdated Show resolved Hide resolved
@truth-quark
Copy link
Collaborator Author

I've refactored the tests with fixtures, now the assertions test against a cube.coord("name") function call interface. It has resulted in some 1st class object passing, but it's constrained within a fixture. There's a potential future benefit as the fixture dataclass can be called with init args in tests.

@blimlim How do you find the readability of this change?

Copy link
Collaborator

@blimlim blimlim left a comment

Choose a reason for hiding this comment

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

Changes look good and clearer to read!

@truth-quark truth-quark merged commit 70a890c into main Aug 29, 2024
4 checks passed
@truth-quark truth-quark deleted the 53/fix-level-coord branch October 1, 2024 06:44
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.

Add tests for fix_level_coord()
2 participants