Skip to content

Unify and deduplicate float tests #141726

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

Open
RalfJung opened this issue May 29, 2025 · 8 comments
Open

Unify and deduplicate float tests #141726

RalfJung opened this issue May 29, 2025 · 8 comments
Labels
A-floating-point Area: Floating point numbers and arithmetic A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented May 29, 2025

We currently have at least two somewhat independent places where float functions are being tested:

  • in coretests/tests/num/mod.rs
  • in coretests/tests/floats/f*.rs
  • in std/tests/floats/f*.rs

Confusing, this also tests things that are actually in std. The ones in num have infrastructure to also run tests in const, but do not have infrastructure to e.g. only run f128 tests on targets where that is reliable. For this reason, some tests are duplicated.

We should make sure we have one canonical place for all these tests that can serve the needs of all of them, and avoid duplication.

Cc @tgross35

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 29, 2025
@jieyouxu jieyouxu added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-testsuite Area: The testsuite used to check the correctness of rustc T-libs Relevant to the library team, which will review and decide on the PR/issue. A-floating-point Area: Floating point numbers and arithmetic and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 29, 2025
@RalfJung
Copy link
Member Author

I prototyped a macro that would let us write tests like this:

#[test]
fn test_min() {
    float_test!(
        for Float in (
            #[cfg(target_has_reliable_f16_math)] f16,
            f32, f64,
            #[cfg(target_has_reliable_f128_math)] #[cfg(not(miri))] f128,
            const { f16, f32, f64, f128, },
        ) {
            float_assert!((0.0 as Float).min(0.0), 0.0);
        }
    );
}

See the playground for sources.

@tgross35 what do you think?

@bjorn3
Copy link
Member

bjorn3 commented May 29, 2025

Maybe pull the float_test!() macro out of the test function? So in case of a test failure, you get separate test entries for the different float types. Could be useful to know if you broke floats as a whole or just f16 or f128 floats.

@RalfJung
Copy link
Member Author

RalfJung commented May 29, 2025

Hm yeah that should work:

float_test!(min:
    for Float in (
        #[cfg(target_has_reliable_f16_math)] f16,
        f32, f64,
        #[cfg(target_has_reliable_f128_math)] #[cfg(not(miri))] f128,
        const { f16, f32, f64, f128, },
    ) {
        float_assert!((0.0 as Float).min(0.0), 0.0);
    }
);

Output:

running 7 tests
test min::const_::f128 ... ok
test min::const_::f16 ... ok
test min::const_::f32 ... ok
test min::const_::f64 ... ok
test min::f16 ... ok
test min::f32 ... ok
test min::f64 ... ok

Full code

@tgross35
Copy link
Contributor

Something like that seems reasonable enough to me. The config is a bit messy to read, but I don't have any better proposals.

Instead of float_assert!, could we create a module defining const-usable assert, assert_eq, assert_biteq, and assert_approx_eq macros, then bring those into scope for the const tests? Seems easier than going through a single float_assert!.

Also note that changes here may race with #141669, hopefully that one can get in first.

@RalfJung
Copy link
Member Author

Instead of float_assert!, could we create a module defining const-usable assert, assert_eq, assert_biteq, and assert_approx_eq macros, then bring those into scope for the const tests? Seems easier than going through a single float_assert!.

Modules and macros interact in funny ways, but this variant seems to work.

I made it so that the entire test is put in a const block for the const versions. This has the advantage that if you use a macro that does not have a special const version, it'll still execute at const-time properly. It has the disadvantage that failing const tests fail when the test is being built, not when it is executed.

Also note that changes here may race with #141669, hopefully that one can get in first.

I intended to wait for some other stuff anyway before making an attempt here -- so I can just wait for that PR as well.

@RalfJung
Copy link
Member Author

Turns out we have even more tests in library/std/tests/floats. Ideally we'd share the testing infrastructure

@tgross35
Copy link
Contributor

As a possible alternative, I prototyped something that always emits tests for all float types so we don't accidentally forget any, but allows adding specific config.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=e03c7085c27cd29207d6ee40adda8e05

#[cfg(test)]
float_test! {
    name: min,
    attrs: {
        f16: [cfg(target_has_reliable_f16_math)],
        // no f64 tests for whatever reason
        f64: [cfg(false)],
        const f64: [cfg(false)],
        // Not implemented with miri
        f128: [cfg(target_has_reliable_f128_math), cfg(miri)],
        const f128: [cfg(target_has_reliable_f128_math), cfg(miri)],
    },
    tests: {
        assert!(!Float::NAN.min(0.0).is_nan(), "unexpected nan");
        assert_eq!((0.0 as Float).min(0.0), 0.0, "hi");
    },
}

Turns out we have even more tests in library/std/tests/floats. Ideally we'd share the testing infrastructure

Hopefully more of that will move to core in the future anyway :)

@RalfJung
Copy link
Member Author

As a possible alternative, I prototyped something that always emits tests for all float types so we don't accidentally forget any, but allows adding specific config.

The way that works, if I use any other test macro those tests will not actually be run at const-time ever. (But that's orthogonal to always emitting all types vs iterating over the list of types.)

I figured iterating over the list would be needed to make this handle all cases, but I like your approach with cfg(false). IIUC it will default to just running the tests for all of them, and one has to opt-out rather than opt-in? That's neat.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 4, 2025
coretests: move float tests from num to floats module and use a more flexible macro to generate them

This makes some progress on rust-lang#141726 by moving the float tests in `num` to `floats` and using a newer, more flexible macro to generate them. We also newly run these tests on f16 and f128 in const, and at runtime in Miri and for hosts where that works well enough.

I didn't yet deduplicate any tests or port the existing `floats::f*` tests to the macro, that can happen in a future PR.
rust-bors bot added a commit that referenced this issue Jun 4, 2025
coretests: move float tests from num to floats module and use a more flexible macro to generate them

This makes some progress on #141726 by moving the float tests in `num` to `floats` and using a newer, more flexible macro to generate them. We also newly run these tests on f16 and f128 in const, and at runtime in Miri and for hosts where that works well enough.

I didn't yet deduplicate any tests or port the existing `floats::f*` tests to the macro, that can happen in a future PR.

<!-- homu-ignore:start -->
<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r? <reviewer name>
-->
<!-- homu-ignore:end -->

try-job: x86_64-gnu-aux
jhpratt added a commit to jhpratt/rust that referenced this issue Jun 7, 2025
coretests: move float tests from num to floats module and use a more flexible macro to generate them

This makes some progress on rust-lang#141726 by moving the float tests in `num` to `floats` and using a newer, more flexible macro to generate them. We also newly run these tests on f16 and f128 in const, and at runtime in Miri and for hosts where that works well enough.

I didn't yet deduplicate any tests or port the existing `floats::f*` tests to the macro, that can happen in a future PR.

try-job: x86_64-gnu-aux
jhpratt added a commit to jhpratt/rust that referenced this issue Jun 7, 2025
coretests: move float tests from num to floats module and use a more flexible macro to generate them

This makes some progress on rust-lang#141726 by moving the float tests in `num` to `floats` and using a newer, more flexible macro to generate them. We also newly run these tests on f16 and f128 in const, and at runtime in Miri and for hosts where that works well enough.

I didn't yet deduplicate any tests or port the existing `floats::f*` tests to the macro, that can happen in a future PR.

try-job: x86_64-gnu-aux
rust-timer added a commit that referenced this issue Jun 7, 2025
Rollup merge of #141857 - RalfJung:coretests-floats, r=tgross35

coretests: move float tests from num to floats module and use a more flexible macro to generate them

This makes some progress on #141726 by moving the float tests in `num` to `floats` and using a newer, more flexible macro to generate them. We also newly run these tests on f16 and f128 in const, and at runtime in Miri and for hosts where that works well enough.

I didn't yet deduplicate any tests or port the existing `floats::f*` tests to the macro, that can happen in a future PR.

try-job: x86_64-gnu-aux
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-floating-point Area: Floating point numbers and arithmetic A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants