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

Update testing_boiler and unify test helpers #269

Merged
merged 33 commits into from
Sep 5, 2024

Conversation

FreezyLemon
Copy link
Contributor

@FreezyLemon FreezyLemon commented Sep 4, 2024

The unit tests are a bit messy at the moment. Duplicate code, subtly inconsistent behavior (test_case), bad naming, no documentation etc.

I tried to clean it up a bit. This also makes the problems mentioned in #108 easier to spot (ctrl-f "test_exact"). List of changes:

  • Changed some function names:
    • try_create and create_case -> create_ok (dropped some verification but that really shouldn't matter1. If it does, I can change this)
    • bad_create_case -> create_err (new: create_err returns the error, can be used for verification when more specific errors are returned)
    • test_case -> either test_exact or test_relative, depending on old behavior (some implementations used assert_eq, others used assert_relative_eq)
    • test_almost -> test_absolute
    • get_value -> create_and_get
  • Added documentation to the test helpers
  • Improve messages for test failures in common functions
  • Rewrote some #[should_panic] tests to use test_none instead. Reads better IMHO

There are a few smaller things that could help further (re-enabling rustfmt for unit tests, dis-allowing clippy::excessive_precision), but those are less clear-cut, so I kept it to this changeset for now.

Footnotes

  1. The type of verification being done was extremely rudimentary, e.g. let dist = Dist::new(7.5, 3.5); and then assert_eq!(dist.a(), 7.5); plus assert_eq!(dist.b(), 3.5);.

Example message:
Gamma::new was expected to succeed, but failed for shape=10.0, rate=NaN with error: 'Bad distribution parameters'
Example message:
StudentsT::new was expected to fail, but succeeded for location=0.0, scale=10.0, freedom=1.0 with result: StudentsT { location: 0.0, scale: 10.0, freedom: 1.0 }
`test_case` -> `test_relative`
`test_case_special` -> `test_absolute`

Also improved the error messages
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.30%. Comparing base (aa276c8) to head (110fa86).
Report is 37 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #269      +/-   ##
==========================================
- Coverage   90.62%   89.30%   -1.33%     
==========================================
  Files          50       50              
  Lines       11583    10976     -607     
==========================================
- Hits        10497     9802     -695     
- Misses       1086     1174      +88     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@FreezyLemon
Copy link
Contributor Author

Just noticed I missed some #[should_panic] tests that I still want to rewrite. And I guess I can also write some tests for the testing_boiler itself, now that it's being used for almost all unit tests

@FreezyLemon
Copy link
Contributor Author

The changed lines have 100% coverage, but total coverage still went down because the PR removes circa 700 lines.

@YeungOnion
Copy link
Contributor

I really appreciate this, I think it'll add a lot of value for contributors to have a consistent approach for tests.

I've no qualms with choices of names. I believe at this point, all arguments in new methods are stored in the struct. If that changes (sim to #198), then tests should be added with it.

I've reviewed the minimal functional changes to the boiler and I think it's fair to specify f64 until we actually implement over generic or multiple floats. These error messages will be more specific now as well, thanks 👍!


I believe that the only way this PR could allow behavior of the library to change in the future is if tests aren't the same, so my plan is to do some semantic diff and make sure everything in a pre-existing test module is a rename (or deletion, in the cases you footnote).

Sound reasonable?

@FreezyLemon
Copy link
Contributor Author

Yeah, of course.

Copy link
Contributor

@YeungOnion YeungOnion left a comment

Choose a reason for hiding this comment

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

reviewed locally, looks good!

@YeungOnion YeungOnion merged commit c428c75 into statrs-dev:master Sep 5, 2024
7 of 8 checks passed
@FreezyLemon FreezyLemon deleted the update-testing-boiler branch September 5, 2024 16:09
@FreezyLemon
Copy link
Contributor Author

I think the auto-merge broke something here, master doesn't build atm.

@FreezyLemon
Copy link
Contributor Author

See #277.

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