Handle tests with NIST data as integration tests

statrs-dev / statrs

Statistical computation library for Rust

https://docs.rs/statrs/latest/statrs/

MIT License

544 stars 78 forks source link

Handle tests with NIST data as integration tests #230

Closed YeungOnion closed 1 week ago

YeungOnion commented 1 month ago

Unsure how best to handle these tests, but I think separating them is a good start to not have the data in the repo per #195.

Test suite will rely on environment variable, "STATRS_NIST_DATA_DIR" set as const NIST_DATA_DIR_ENV, to specify path where the files, some listed below, are located.

lew.txt
mavro.txt
numacc1.txt
...

Note: I am not married to the usage of an environment variable or its name.

codecov[bot] commented 1 month ago

Codecov Report

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

Project coverage is 90.10%. Comparing base (49a5209) to head (9d88e8a). Report is 203 commits behind head on master.

:exclamation: Current head 9d88e8a differs from pull request most recent head 494779f

Please upload reports for the commit 494779f to get more accurate results.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #230 +/- ## ========================================== - Coverage 91.76% 90.10% -1.66% ========================================== Files 44 49 +5 Lines 7360 10230 +2870 ========================================== + Hits 6754 9218 +2464 - Misses 606 1012 +406 ```

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

FreezyLemon commented 1 week ago

I like the approach. Maybe add a helper script (bash?) to download the NIST files? Downloading the files manually could be a bit tedious.

YeungOnion commented 1 week ago

We didn't distribute the integration tests with the last release, but if we did, would we want to share the script along with these tests?

FreezyLemon commented 1 week ago

I would logically see the script as a part of that test suite, so yeah.

YeungOnion commented 1 week ago

Will add to CI in another PR, any feedback on this one?

FreezyLemon commented 1 week ago

I tried it on my machine and the script looks good. I think the environment variable works well, no reason to overthink this for a test IMO.

Few ideas to make the experience a bit smoother:

Add tests/*.dat to the .gitignore so the downloaded files don't show up in git
Add a bit of output to the script (e.g. Downloading <file>..., Processing <file>...) so we know what's going on. This gets more important when .gitignore includes the data files
Short comment (top of the nist_tests.rs) that explains how you're intended to actually run the tests would be nice. cargo test -- --include-ignored nist is not obvious IMHO
Use the current working dir when STATRS_NIST_DATA_DIR is unset. That'd make it easier to cd tests; ./gather_nist_data.sh; cargo test without requiring the env variable

YeungOnion commented 1 week ago

Thanks for the feedback, think it will be clearer and nicer. Opted for default target directory to be tests since manifest and ignore target it and set some sense of working directory.