test-float-parse: Use an overrideable random seed#152598
Open
tgross35 wants to merge 3 commits intorust-lang:mainfrom
Open
test-float-parse: Use an overrideable random seed#152598tgross35 wants to merge 3 commits intorust-lang:mainfrom
tgross35 wants to merge 3 commits intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
Collaborator
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
tgross35
force-pushed
the
test-float-parse-updates
branch
from
e82bcb2 to
5939d5d
Compare
This comment has been minimized.
This comment has been minimized.
Currently we warn if `debug_assertions` is set, but this isn't always accurate because debug assertions can be on at higher optimization levels. Forward some information from the build script and print it to tell a better story, and update the warning to act on opt-level and profile rather than `debug_assertions`.
Currently `test-float-parse` is mostly random tests which take a fixed seed, meaning the same set of values get tested for each invocation. This isn't ideal because we don't get the true benefit of randomness, which is to have better coverage over time. Improve this by using a randomly generated seed, which can also be set via env. The seed is printed at the beginning of each run so it is easy to reproduce failures using the same test set, if needed. Typically it isn't great to have fuzzing randomness in tests that get run in CI because it can lead to spurious failures. However, this is a test for which failure should never happen becasue the algorithms are reasonably well proven, so if one does occur it represents a very unexpected bug that needs to be addressed. The error message is updated to strongly recommend reporting before retrying and to include details on how to reproduce.
tgross35
force-pushed
the
test-float-parse-updates
branch
from
5939d5d to
b99607f
Compare
Contributor
Author
|
The new version of indicatif pulls in a new wasm dep tree, which we don't really need. Looks like they made these optional in 0.18.4 but it hasn't been released yet console-rs/indicatif#766, so can probably just wait for that. |
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Mark-Simulacrum
approved these changes
Feb 14, 2026
| fn main() { | ||
| // Forward the opt level so we can warn if the tests are going to be slow. | ||
| let opt = env::var("OPT_LEVEL").expect("OPT_LEVEL unset"); | ||
| let profile = env::var("PROFILE").expect("PROFILE unset"); |
Member
There was a problem hiding this comment.
Do we need to tell Cargo that we read these environment variables, or is that assumed?
| const SEED: [u8; 32] = *b"3.141592653589793238462643383279"; | ||
| const SEED_ENV: &str = "TEST_FLOAT_PARSE_SEED"; | ||
|
|
||
| /// Seed for tests that use a deterministic RNG, and its b64 representation for printing. Taken |
Member
There was a problem hiding this comment.
Maybe save the dependency and just hex-dump? Should be fairly easy with something like {:#032x}{:032x} with u128...
Contributor
|
☔ The latest upstream changes (presumably #152934) made this pull request unmergeable. Please resolve the merge conflicts. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently
test-float-parseis mostly random tests which take a fixedseed, meaning the same set of values get tested for each invocation.
This isn't ideal because we don't get the true benefit of randomness,
which is to have better coverage over time.
Improve this by using a randomly generated seed, which can also be set
via env. The seed is printed at the beginning of each run so it is easy
to reproduce failures using the same test set, if needed.
Typically it isn't great to have fuzzing randomness in tests that get
run in CI because it can lead to spurious failures. However, this is a
test for which failure should never happen becasue the algorithms are
reasonably well proven, so if one does occur it represents a very
unexpected bug that needs to be addressed. The error message is updated
to strongly recommend reporting before retrying and to include details
on how to reproduce.
Additionally there are two other commits with less notable changes,
details are in the messages there.