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

Test failures pre 3.8.0 #10474

Closed
zanchey opened this issue May 2, 2024 · 7 comments · Fixed by #10582
Closed

Test failures pre 3.8.0 #10474

zanchey opened this issue May 2, 2024 · 7 comments · Fixed by #10582
Labels
regression Something that used to work, but was broken, especially between releases
Milestone

Comments

@zanchey
Copy link
Member

zanchey commented May 2, 2024

The tests are failing on the package builders in a few places.

I'm filing an issue for posterity and also to make sure they actually get fixed.

  1. 32-bit builds fail in src/tests/common.rs:127:
assertion `left == right` failed
  left: "-9223372036854775808"
 right: "0"

@KamilaBorowska spotted this one as the argument to sprintf being long, which is 32 bits on a 32-bit platform rather than defined as 64-bit.

  1. 32-bit builds fail in src/wutil/wcstod.rs:575:9
thread 'wutil::wcstod::test::tests' panicked at 'assertion failed: `(left == right)`
 left: `Ok(8.925500000000001e-18)`,
 right: `Ok(8.9255e-18)`'

@mqudsi notes this is the wrong comparison to be doing as floating point values should be compared by epsilon

I don't know why these two aren't picked up by the 32-bit CI build.

  1. Unwritable home directories make the history tests fail.
---- tests::history::test_history_formats stdout ----
thread 'tests::history::test_history_formats' panicked at src/tests/history.rs:561:32:
Failed to get data directory

---- tests::history::test_history_merge stdout ----
thread 'tests::history::test_history_merge' panicked at src/tests/history.rs:395:9:
assertion failed: history_contains(&everything, text)

---- tests::history::test_history_races stdout ----
thread 'tests::history::test_history_races' panicked at src/tests/history.rs:336:5:
assertion `left == right` failed
  left: 1
 right: 1025

HOME is set to something unwritable in the Debian package builds. This wasn't a problem pre-cargo test, because tests/test_env.sh was run beforehand. The cargo tests should be equally hermetic. It probably means reimplementing test_env.sh in Rust, because sourcing it before running cargo test stuffs up any rustup-installed toolchains.

@zanchey zanchey added this to the fish next-3.x milestone May 2, 2024
@zanchey zanchey added the regression Something that used to work, but was broken, especially between releases label May 2, 2024
@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented May 2, 2024

Testing for equality in wcstod tests is actually correct, this is pointing a genuine problem. However, this is specifically caused by x87 floating point being pretty broken: rust-lang/rust#114479.

This problem affects Rust's standard library too. Rust's standard library has a pretty cursed workaround for this issue: https://github.com/rust-lang/rust/blob/master/library/core/src/num/dec2flt/fpu.rs.

Personally I would skip those tests for non-SSE2 x86 all(target_arch = "x86", not(target_feature = "sse2")).

Or alternatively, use str::parse rather than fast_float for those targets instead (as standard library has a workaround for this bug). That said, I don't think it's worth it, x87 is not IEEE compliant, so the results are going to be subtly wrong when doing mathematical operations anyway.

mqudsi added a commit that referenced this issue May 5, 2024
%ld expects a 4-byte parameter on 32-bit architectures and an 8-byte parameter
on 64-bit architectures, but we supplied are trying to supply a 64-bit parameter
that would overflow 32-bit storage.

Use %lld instead which expects a `long long` parameter, which should be 8-bytes
under both architectures.

See #10474
mqudsi added a commit that referenced this issue May 5, 2024
As documented in #10474, there are issues with 64-bit floating point rounding
under x86 targets without SSE2 extensions, where x87 floating point math causes
imprecise results.

Document the shortcoming and provide some version of the test that passes
regardless of architecture.
@mqudsi
Copy link
Contributor

mqudsi commented May 5, 2024

I pushed some updates that got tests working for me targeting i586-unknown-linux-gnu under Debian 12 i686. Using str::parse() instead of fast_float is not a one-liner given how we use the latter, so I'm just using a combination of my suggested test and your sse2 note.

The history_formats test failure is because we are including a source code test that relies on the cmake build/tests target being present. I'll fix that separately.

@mqudsi
Copy link
Contributor

mqudsi commented May 5, 2024

test_history_formats fixed in 476b360; everything should be passing now (EDIT: but that's locally, not under CI where the permissions issue exists separately!)

mqudsi added a commit that referenced this issue May 6, 2024
We will continue to use the "normal" fish base directory detection when using
the CMake test harness which properly sets up a sandboxed $HOME for fish to use,
but when running source code tests with a bare `cargo test` we don't want to
write to the actual user's profile.

This also works around test failures when running `cargo test` under CI with a
locked-down $HOME directory (see #10474).
@mqudsi
Copy link
Contributor

mqudsi commented May 6, 2024

@zanchey after a35925b, tests appear to pass with the following under Debian 12 i686, which I assume does the trick for you?

set fish_home (mktemp -d)
sudo chown root:root $fish_home
env HOME=$fish_home cargo test --target i586-unknown-linux-gnu

(I don't actually run the last command as-is because cargo test actually calls rustup which uses $HOME for its own purposes; I actually run a bare cargo test --target i586-unknown-linux-gnu once which builds the test binary as target/debug/i586-unknown-linux-gnu/fish-<hexadecimal-string> then I run that binary with env HOME=$fish_home.)

@mqudsi
Copy link
Contributor

mqudsi commented May 7, 2024

@KamilaBorowska do you have any idea why the other 32-bit vs 64-bit errors didn't manifest under i686 but show up under i586? For example, I would expect the snprintf issue with %ld passed an i64 to show up regardless of which particular 32-bit flavor we're targeting.

@zanchey
Copy link
Member Author

zanchey commented May 18, 2024

This is all working ok now, and I fixed the issues with Ubuntu's lto-wrapper (which breaks for some reason when linking the cargo tests) with f4a79cc.

@zanchey zanchey closed this as completed May 18, 2024
@zanchey
Copy link
Member Author

zanchey commented Jun 12, 2024

Unfortunately the 32-bit builds are failing on Debian again.

Testing file checks/math.fish ... Failure:

The CHECK on line 77 wants:
  1.316074

which failed to match line stdout:32:
  1.316075

Context:
  [...] from line 71 (stdout:29):
  1000000000000000
  100000000000000
  -1000000000000000
  1.316075 <= does not match CHECK on line 77: 1.316074
  4
  0
  1
  [...] from line 261 (stdout:77):
  2
  3
  0.841471
  -0.89001 <= does not match CHECK on line 283: -0.890009
  0.5
  4
  2

when running command:
  ../test/root/bin/fish checks/math.fish

and

---- tests::negative_precision_width stdout ----
thread 'tests::negative_precision_width' panicked at 'assertion failed: `(left == right)`
  left: `"78.900001   "`,
 right: `"78.900000   "`', printf/src/tests.rs:601:5

---- tests::test_float stdout ----
thread 'tests::test_float' panicked at 'assertion failed: `(left == right)`
  left: `"1.100001"`,
 right: `"1.100000"`', printf/src/tests.rs:359:5

---- tests::test_float_g stdout ----
thread 'tests::test_float_g' panicked at 'assertion failed: `(left == right)`
  left: `"1.23456789012346"`,
 right: `"1.23456789012345"`', printf/src/tests.rs:453:5

---- tests::test_locale stdout ----
thread 'tests::test_locale' panicked at 'assertion failed: `(left == right)`
  left: `"-46,380001"`,
 right: `"-46,380000"`', printf/src/tests.rs:702:9

@zanchey zanchey reopened this Jun 12, 2024
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 21, 2024
Due to the inherent floating point accuracy issues under i586 described
in fish-shell#10474 and at rust-lang/rust#114479, we need to add
a workaround to our littlecheck math tests to perform less stringent comparisons
when fish was built for i586 (*not* i686, so we don't check `status target-arch`
but rather `status target` with a prefix match).

To reduce the likelihood of inadvertent test breakage, the i586 reduced-accuracy
tests are always run, but we also run our "standard" test for other
architectures.

This commit addresses the littlecheck issues that caused fish-shell#10474 to be re-opened,
but I still have to reproduce the cargo test failures for
`negative_precision_width`, `test_float`, `test_float_g`, and `test_locale`.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 21, 2024
Due to the inherent floating point accuracy issues under i586 described
in fish-shell#10474 and at rust-lang/rust#114479, we need to add
a workaround to our littlecheck math tests to perform less stringent comparisons
when fish was built for i586 (*not* i686, so we don't check `status target-arch`
but rather `status target` with a prefix match).

To reduce the likelihood of inadvertent test breakage, the i586 reduced-accuracy
tests are always run, but we also run our "standard" test for other
architectures.

This commit addresses the littlecheck issues that caused fish-shell#10474 to be re-opened,
but I still have to reproduce the cargo test failures for
`negative_precision_width`, `test_float`, `test_float_g`, and `test_locale`.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 21, 2024
A few specific tests fail under i586 due to its inherent floating point
inaccuracy issues (rust-lang/rust#114479), so ignore these tests if certain
are met.

We have specific integration tests elsewhere in fish to check that even under
i586 we get mostly sane results, so this is OK. I tried to modify the assert
macros to check for a loose string match (up to one character difference) or an
f64 abs diff of less than epsilon, but it was a lot of code with little value
and increased the friction to contributing to the tests. Also, let's just
acknowledge the fact that all of i686, let alone i586 specifically, is a dead
end and not worth investing such time and effort into so long as it more or less
"works".

Closes fish-shell#10474.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 21, 2024
Due to the inherent floating point accuracy issues under i586 described
in fish-shell#10474 and at rust-lang/rust#114479, we need to add
a workaround to our littlecheck math tests to perform less stringent comparisons
when fish was built for i586 (*not* i686, so we don't check `status target-arch`
but rather `status target` with a prefix match).

To reduce the likelihood of inadvertent test breakage, the i586 reduced-accuracy
tests are always run, but we also run our "standard" test for other
architectures.

This commit addresses the littlecheck issues that caused fish-shell#10474 to be re-opened,
but I still have to reproduce the cargo test failures for
`negative_precision_width`, `test_float`, `test_float_g`, and `test_locale`.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 21, 2024
A few specific tests fail under i586 due to its inherent floating point
inaccuracy issues (rust-lang/rust#114479), so ignore these tests if certain
are met.

We have specific integration tests elsewhere in fish to check that even under
i586 we get mostly sane results, so this is OK. I tried to modify the assert
macros to check for a loose string match (up to one character difference) or an
f64 abs diff of less than epsilon, but it was a lot of code with little value
and increased the friction to contributing to the tests. Also, let's just
acknowledge the fact that all of i686, let alone i586 specifically, is a dead
end and not worth investing such time and effort into so long as it more or less
"works".

Closes fish-shell#10474.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 21, 2024
Due to the inherent floating point accuracy issues under i586 described
in fish-shell#10474 and at rust-lang/rust#114479, we need to add
a workaround to our littlecheck math tests to perform less stringent comparisons
when fish was built for i586 (*not* i686, so we don't check `status target-arch`
but rather `status target` with a prefix match).

To reduce the likelihood of inadvertent test breakage, the i586 reduced-accuracy
tests are always run, but we also run our "standard" test for other
architectures.

This commit addresses the littlecheck issues that caused fish-shell#10474 to be re-opened,
but I still have to reproduce the cargo test failures for
`negative_precision_width`, `test_float`, `test_float_g`, and `test_locale`.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 21, 2024
A few specific tests fail under i586 due to its inherent floating point
inaccuracy issues (rust-lang/rust#114479), so ignore these tests if certain
are met.

We have specific integration tests elsewhere in fish to check that even under
i586 we get mostly sane results, so this is OK. I tried to modify the assert
macros to check for a loose string match (up to one character difference) or an
f64 abs diff of less than epsilon, but it was a lot of code with little value
and increased the friction to contributing to the tests. Also, let's just
acknowledge the fact that all of i686, let alone i586 specifically, is a dead
end and not worth investing such time and effort into so long as it more or less
"works".

Closes fish-shell#10474.
@mqudsi mqudsi linked a pull request Jun 21, 2024 that will close this issue
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 22, 2024
Due to the inherent floating point accuracy issues under i586 described
in fish-shell#10474 and at rust-lang/rust#114479, we need to add
a workaround to our littlecheck math tests to perform less stringent comparisons
when fish was built for x86 without SSE2 support. This normally means checking
for an i586 target triple, but some distros like Debian patch the i686 target to
strip out SSE2 support as well, so we need to use a combination of a target
architecture check (for x86) combined with a runtime test to check for broken
floating point precision.

To reduce the likelihood of inadvertent test breakage, these reduced-accuracy
tests are always run, but we also run our "standard" test for other
architectures.

This commit addresses the littlecheck issues that caused fish-shell#10474 to be re-opened,
but I still have to reproduce the cargo test failures for
`negative_precision_width`, `test_float`, `test_float_g`, and `test_locale`.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 22, 2024
A few specific tests fail under i586 due to its inherent floating point
inaccuracy issues (rust-lang/rust#114479), so ignore these tests if certain
are met.

We have specific integration tests elsewhere in fish to check that even under
i586 we get mostly sane results, so this is OK. I tried to modify the assert
macros to check for a loose string match (up to one character difference) or an
f64 abs diff of less than epsilon, but it was a lot of code with little value
and increased the friction to contributing to the tests. Also, let's just
acknowledge the fact that all of i686, let alone i586 specifically, is a dead
end and not worth investing such time and effort into so long as it more or less
"works".

Closes fish-shell#10474.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 23, 2024
Due to the inherent floating point accuracy issues under i586 described
in fish-shell#10474 and at rust-lang/rust#114479, we need to add
a workaround to our littlecheck math tests to perform less stringent comparisons
when fish was built for x86 without SSE2 support.

This commit addresses the littlecheck issues that caused fish-shell#10474 to be re-opened,
but I still have to reproduce the cargo test failures for
`negative_precision_width`, `test_float`, `test_float_g`, and `test_locale`.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 23, 2024
A few specific tests fail under i586 due to its inherent floating point
inaccuracy issues (rust-lang/rust#114479), so ignore these tests if certain
are met.

We have specific integration tests elsewhere in fish to check that even under
i586 we get mostly sane results, so this is OK. I tried to modify the assert
macros to check for a loose string match (up to one character difference) or an
f64 abs diff of less than epsilon, but it was a lot of code with little value
and increased the friction to contributing to the tests. Also, let's just
acknowledge the fact that all of i686, let alone i586 specifically, is a dead
end and not worth investing such time and effort into so long as it more or less
"works".

Closes fish-shell#10474.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 23, 2024
Due to the inherent floating point accuracy issues under i586 described
in fish-shell#10474 and at rust-lang/rust#114479, we need to add
a workaround to our littlecheck math tests to perform less stringent comparisons
when fish was built for x86 without SSE2 support.

This commit addresses the littlecheck issues that caused fish-shell#10474 to be re-opened,
but I still have to reproduce the cargo test failures for
`negative_precision_width`, `test_float`, `test_float_g`, and `test_locale`.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jun 23, 2024
A few specific tests fail under i586 due to its inherent floating point
inaccuracy issues (rust-lang/rust#114479), so ignore these tests if certain
are met.

We have specific integration tests elsewhere in fish to check that even under
i586 we get mostly sane results, so this is OK. I tried to modify the assert
macros to check for a loose string match (up to one character difference) or an
f64 abs diff of less than epsilon, but it was a lot of code with little value
and increased the friction to contributing to the tests. Also, let's just
acknowledge the fact that all of i686, let alone i586 specifically, is a dead
end and not worth investing such time and effort into so long as it more or less
"works".

Closes fish-shell#10474.
mqudsi added a commit that referenced this issue Jun 23, 2024
Due to the inherent floating point accuracy issues under i586 described
in #10474 and at rust-lang/rust#114479, we need to add
a workaround to our littlecheck math tests to perform less stringent comparisons
when fish was built for x86 without SSE2 support.

This commit addresses the littlecheck issues that caused #10474 to be re-opened,
but I still have to reproduce the cargo test failures for
`negative_precision_width`, `test_float`, `test_float_g`, and `test_locale`.
mqudsi added a commit that referenced this issue Jun 23, 2024
A few specific tests fail under i586 due to its inherent floating point
inaccuracy issues (rust-lang/rust#114479), so ignore these tests if certain
are met.

We have specific integration tests elsewhere in fish to check that even under
i586 we get mostly sane results, so this is OK. I tried to modify the assert
macros to check for a loose string match (up to one character difference) or an
f64 abs diff of less than epsilon, but it was a lot of code with little value
and increased the friction to contributing to the tests. Also, let's just
acknowledge the fact that all of i686, let alone i586 specifically, is a dead
end and not worth investing such time and effort into so long as it more or less
"works".

Closes #10474.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Something that used to work, but was broken, especially between releases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants