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

CI failure - floating point calculation assertions #48

Closed
jnqnfe opened this issue Feb 1, 2022 · 6 comments
Closed

CI failure - floating point calculation assertions #48

jnqnfe opened this issue Feb 1, 2022 · 6 comments

Comments

@jnqnfe
Copy link
Owner

jnqnfe commented Feb 1, 2022

I'm aware that with the latest automated monthly CI run the instance using the nightly compiler is now failing, as I can reproduce locally with the nightly compiler.

The following tests are failing:

  • src/time/microseconds.rs - time::microseconds::MicroSeconds::div_f32 (line 595)
  • src/time/microseconds.rs - time::microseconds::MicroSeconds::from_secs_f32 (line 257)
  • src/time/microseconds.rs - time::microseconds::MicroSeconds::from_secs_f64 (line 218)
  • src/time/microseconds.rs - time::microseconds::MicroSeconds::mul_f32 (line 508)

The error output is as follows:

---- src/time/microseconds.rs - time::microseconds::MicroSeconds::div_f32 (line 595) stdout ----
Test executable failed (exit code 101).

stderr:
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `MicroSeconds(859872558)`,
 right: `MicroSeconds(859872559)`', src/time/microseconds.rs:7:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


---- src/time/microseconds.rs - time::microseconds::MicroSeconds::from_secs_f32 (line 257) stdout ----
Test executable failed (exit code 101).

stderr:
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `MicroSeconds(2299999)`,
 right: `MicroSeconds(2300000)`', src/time/microseconds.rs:6:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


---- src/time/microseconds.rs - time::microseconds::MicroSeconds::from_secs_f64 (line 218) stdout ----
Test executable failed (exit code 101).

stderr:
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `MicroSeconds(2299999)`,
 right: `MicroSeconds(2300000)`', src/time/microseconds.rs:6:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


---- src/time/microseconds.rs - time::microseconds::MicroSeconds::mul_f32 (line 508) stdout ----
Test executable failed (exit code 101).

stderr:
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `MicroSeconds(8478000000)`,
 right: `MicroSeconds(8478000152)`', src/time/microseconds.rs:8:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I'll head now to the Rust compiler issue list to file an issue report...

@werdahias
Copy link

I also have that bug when trying to package the latest libpulse-binding for debian. If I run the test they fail because of ~1 ms time differences. Would you consider lowering the precision for those tests ?

@werdahias
Copy link

Reading up on the linked issue:
Maybe implement it like that:

use float_eq::float_eq;


float_eq!(comparing ms here);

I don't know enough rust to write a suitable patch tbh.

jnqnfe added a commit that referenced this issue Jan 9, 2023
version 1.60 of the rust stdlib included a change to improve the accuracy
of calculations in `Duration::try_from_secs_f32/64` (see [1]), which had a
minor breaking change of slightly altering the calculated results. version
1.63 further tweaked this (see [2]).

this commit updates tests to match the new calculated values from 1.63+,
and thus fixes failure running tests in newer stable rust releases and
helps us to fix CI failure. this is done at the expense of breaking the
ability to run the tests with older rust versions, which seems reasonably
acceptable at this time. for the record, the new values used here are
those calculated using version 1.66.

it has been suggested that rather than compare float values directly, it
may be better for this crate to compare via the `float_eq` crate instead.
this would essentially mean switching to a less accurate comparison. since
some time has passed now since i originally encountered this problem, i
think that just updating the values compared against is the best move at
this time, though i am not necessarily against moving to use of `float_eq`
at some point in future if this causes significant problems for users of
older rust versions, or if we encounter further such issues from rust
stdlib changes.

note that in the original issue (see [3]) there were four functions whose
doc-tests were failing, yet only three have needed to be modified here.
the lack of needing to fix the fourth function's tests is presumably as a
result of the changes made in rust v1.63. (the original issue was noticed
with a v1.60 nightly).

[1]: rust-lang/rust#90247
[2]: rust-lang/rust#96051
[3]: #48
@jnqnfe
Copy link
Owner Author

jnqnfe commented Jan 9, 2023

Hi, I've now published a new version which should fix this.

I've explained things in the bug report (edit: I meant the commit log), but to update the report - essentially, rust v1.60 included a minor breaking change to the way float-point calculations were done in certain stdlib functions, and this got tweaked again in version 1.63.

At the time I first noticed this, with failures from a 1.60 nightly, it was suggested that I should make use of float_eq to perform comparisons for the test, but I wasn't entirely comfortable with that and ended up just leaving it for some time, for which I apologise.

Since we've moved on several stable versions now, I felt that it was best to simply adjust the tests according to the values expected from rust stdlib v1.63+. Thus flipping the situation such that cargo test now succeeds for rust versions 1.63 and newer, but fails for older versions, rather than the opposite.

I noticed that rustc in debian is at 1.63, so this should pose no issue to these crates making their way into Debian, which would be awesome btw! :D

If there happens to be any users of an older rust version for whom this is a problem, we can revisit the float_eq thing, and similarly we can reconsider it if such a situation with rust's stdlib ever occurs again (I'm still not entirely comfortable with making use of float_eq here and would rather not implement use of it just to future proof against a hypothetical further breaking of tests at some point in the future that may never occur).

@jnqnfe jnqnfe closed this as completed Jan 9, 2023
@jnqnfe
Copy link
Owner Author

jnqnfe commented Jan 9, 2023 via email

@werdahias
Copy link

thanks for all the helpful hints :) I just decided to skip those two tests as getting them to run would have been too much effort and skipped them. libpulse-binding is just awaiting upload :)

@jnqnfe
Copy link
Owner Author

jnqnfe commented Jan 9, 2023 via email

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

No branches or pull requests

2 participants