statrs-dev / statrs

Statistical computation library for Rust
https://docs.rs/statrs/latest/statrs/
MIT License
603 stars 84 forks source link

Update testing_boiler and unify test helpers #269

Closed FreezyLemon closed 2 months ago

FreezyLemon commented 2 months ago

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:

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.

[^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);.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

FreezyLemon commented 2 months ago

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 commented 2 months ago

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

YeungOnion commented 2 months ago

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 commented 2 months ago

Yeah, of course.

FreezyLemon commented 2 months ago

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

FreezyLemon commented 2 months ago

See #277.