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

Streamline the way that test iteration count is determined (replace NTESTS) #379

Merged
merged 5 commits into from
Jan 6, 2025

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Dec 29, 2024

Currently, tests use a handful of constants to determine how many iterations to perform: NTESTS, AROUND, and MAX_CHECK_POINTS. This configuration is not very straightforward to adjust and needs to be repeated everywhere it is used.

Replace this with new functions in the run_cfg module that determine iteration counts in a more reusable and documented way.

This also changes the random generator to generate inputs on the fly rather than caching.

@tgross35 tgross35 force-pushed the tgross35/ntests branch 7 times, most recently from 858a56c to 4f2b124 Compare December 30, 2024 03:45
@tgross35 tgross35 changed the title Replace NTESTS with a better configuration Streamline the way that test iteration count is determined (replace NTESTS) Dec 30, 2024
@tgross35 tgross35 force-pushed the tgross35/ntests branch 5 times, most recently from 8dc619a to 7f851d9 Compare December 30, 2024 04:07
@tgross35 tgross35 marked this pull request as ready for review December 30, 2024 04:08
@tgross35
Copy link
Contributor Author

@beetrees if you get the chance, could you give this a second set of eyes? Mostly just the rewrite of random.rs and that the logic in run_cfg looks reasonable.

I changed the PR for extensive tests to also build off of this, I'll get that one ready next #364.

Occasionally it is useful to see some information from running tests
without making everything noisy from `--nocapture`. Add a function to
log this kind of output to a file, and print the file as part of CI.
Currently, tests use a handful of constants to determine how many
iterations to perform: `NTESTS`, `AROUND`, and `MAX_CHECK_POINTS`. This
configuration is not very straightforward to adjust and needs to be
repeated everywhere it is used.

Replace this with new functions in the `run_cfg` module that determine
iteration counts in a more reusable and documented way.

This only updates `edge_cases` and `domain_logspace`, `random` is
refactored in a later commit.
Introduce the `KnownSize` iterator wrapper, which allows providing the
size at construction time. This provides an `ExactSizeIterator`
implemenation so we can check a generator's value count during testing.
Currently, all inputs are generated and then cached. This works
reasonably well but it isn't very configurable or extensible (adding
`f16` and `f128` is awkward).

Replace this with a trait for generating random sequences of tuples.
This also removes possible storage limitations of caching all inputs.
@tgross35 tgross35 force-pushed the tgross35/ntests branch 3 times, most recently from 2d8b0d4 to c93054b Compare January 6, 2025 00:57
@tgross35
Copy link
Contributor Author

tgross35 commented Jan 6, 2025

Added @quaternic's Zulip suggestion to randomize the seed with a way to override it so we wind up with better test coverage over time.

@tgross35
Copy link
Contributor Author

tgross35 commented Jan 6, 2025

I am going to merge this so I can get other dependent changes in, as always continued reviews are welcome.

@tgross35 tgross35 merged commit b67e65b into master Jan 6, 2025
30 checks passed
@tgross35 tgross35 deleted the tgross35/ntests branch January 6, 2025 02:39
@tgross35 tgross35 restored the tgross35/ntests branch January 6, 2025 02:39
@tgross35 tgross35 deleted the tgross35/ntests branch January 6, 2025 03:13
@beetrees
Copy link
Contributor

beetrees commented Jan 7, 2025

@beetrees if you get the chance, could you give this a second set of eyes? Mostly just the rewrite of random.rs and that the logic in run_cfg looks reasonable.

Apologies for the delay, LGTM.

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