Skip to content

test: Fix failing univalue float test #29892

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.

Sign up for GitHub

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

Jump to bottom
Merged
fanquake merged 1 commit into from
Apr 17, 2024
Merged

test: Fix failing univalue float test #29892

fanquake merged 1 commit into from
Apr 17, 2024

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Apr 16, 2024

Currently the test may fail for some compilers, because 1e-8 may not be possible to represent exactly/consistently.

$ ./src/univalue/test/object 
object: univalue/test/object.cpp:424: void univalue_readwrite(): Assertion `v.read("0.00000000000000000000000000000000000001e+30 ") && v.get_real() == 1e-8' failed.
Aborted (core dumped)

Fixes #27256 (comment)

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 16, 2024 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v, laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Apr 16, 2024
@maflcko maflcko mentioned this pull request Apr 16, 2024
Merged
@fanquake fanquake requested a review from stickies-v April 16, 2024 15:26
stickies-v
stickies-v approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa4c696 , thanks for fixing!

@maflcko
Copy link
Member Author

maflcko commented Apr 16, 2024

For testing, one can use gcc-13 (or later) on i386.

For example, running the centos CI task on fedora:40:

diff --git a/ci/test/00_setup_env_i686_centos.sh b/ci/test/00_setup_env_i686_centos.sh
index 5f8391c..dc80e98 100755
--- a/ci/test/00_setup_env_i686_centos.sh
+++ b/ci/test/00_setup_env_i686_centos.sh
@@ -12,6 +12,7 @@ export CI_IMAGE_NAME_TAG="quay.io/centos/amd64:stream9"
 export CI_BASE_PACKAGES="gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-devel.i686 libstdc++-devel.i686 ccache libtool make git python3 python3-pip which patch lbzip2 xz procps-ng dash rsync coreutils bison util-linux e2fsprogs cmake"
 export PIP_PACKAGES="pyzmq"
 export GOAL="install"
+export DEP_OPTS="NO_QT=1"
 export NO_WERROR=1  # Suppress error: #warning _FORTIFY_SOURCE > 2 is treated like 2 on this platform [-Werror=cpp]
-export BITCOIN_CONFIG="--enable-zmq --with-gui=qt5 --enable-reduce-exports"
+export BITCOIN_CONFIG="--enable-zmq --enable-reduce-exports"
 export CONFIG_SHELL="/bin/dash"
diff --git a/ci/test/01_base_install.sh b/ci/test/01_base_install.sh
index 25962a5..6f61394 100755
--- a/ci/test/01_base_install.sh
+++ b/ci/test/01_base_install.sh
@@ -20,7 +20,6 @@ if [ -n "$DPKG_ADD_ARCH" ]; then
 fi
 
 if [[ $CI_IMAGE_NAME_TAG == *centos* ]]; then
-  bash -c "dnf -y install epel-release"
   bash -c "dnf -y --allowerasing install $CI_BASE_PACKAGES $PACKAGES"
 elif [ "$CI_OS_NAME" != "macos" ]; then
   if [[ -n "${APPEND_APT_SOURCES_LIST}" ]]; then

Or, alternatively, using gcc in the multiprocess task:

diff --git a/ci/test/00_setup_env_i686_multiprocess.sh b/ci/test/00_setup_env_i686_multiprocess.sh
index 00a4d78..517ca0c 100755
--- a/ci/test/00_setup_env_i686_multiprocess.sh
+++ b/ci/test/00_setup_env_i686_multiprocess.sh
@@ -9,10 +9,10 @@ export LC_ALL=C.UTF-8
 export HOST=i686-pc-linux-gnu
 export CONTAINER_NAME=ci_i686_multiprocess
 export CI_IMAGE_NAME_TAG="docker.io/amd64/ubuntu:24.04"
-export PACKAGES="llvm clang g++-multilib"
-export DEP_OPTS="DEBUG=1 MULTIPROCESS=1"
+export PACKAGES="g++-multilib"
+export DEP_OPTS="DEBUG=1 MULTIPROCESS=1 NO_QT=1"
 export GOAL="install"
 export TEST_RUNNER_EXTRA="--v2transport"
-export BITCOIN_CONFIG="--enable-debug CC='clang -m32' CXX='clang++ -m32' \
+export BITCOIN_CONFIG="--enable-debug  \
 CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE'"
 export BITCOIND=bitcoin-node  # Used in functional tests

@laanwj
Copy link
Member

laanwj commented Apr 17, 2024

Right. Direct floating point equality checks are a big nono. It should either use an epsilon comparison, or discretize. Which is what the new test does.

ACK fa4c696

@fanquake fanquake merged commit c8e3b94 into bitcoin:master Apr 17, 2024
@fanquake fanquake mentioned this pull request Apr 17, 2024
Merged
@fanquake
Copy link
Member

fanquake commented Apr 17, 2024

Backported to 27.x in #29888.

@maflcko maflcko deleted the 2404-fix-float-univalue-test- branch April 17, 2024 08:29
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Apr 17, 2024
@fanquake
test: Fix failing univalue float test
9277793 
Github-Pull: bitcoin#29892
Rebased-From: fa4c696
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Apr 24, 2024
@luke-jr
test: Fix failing univalue float test
adcdaea 
Github-Pull: bitcoin#29892
Rebased-From: fa4c696
fanquake added a commit that referenced this pull request May 13, 2024
@fanquake
Merge #29888: [27.x] Backports
c7885ec 
bd5860b [WIP] doc: release notes for 27.x (fanquake)
475aac4 doc: add LLVM instruction for macOS < 13 (Sjors Provoost)
a995902 depends: Fix build of Qt for 32-bit platforms (laanwj)
0fcceef Fix #29767, set m_synced = true after Commit() (nanlour)
ae9a2ed sign: don't assume we are parsing a sane Miniscript (Antoine Poinsot)
a6a59cf rpc: Reword SighashFromStr error message (MarcoFalke)
364bf01 build: Fix false positive `CHECK_ATOMIC` test for clang-15 (Hennadii Stepanov)
9277793 test: Fix failing univalue float test (MarcoFalke)
5c09791 doc: archive 27.0 release notes (fanquake)
897e5af [rpc, bugfix] Enforce maximum value for setmocktime (dergoegge)
602cfd5 ci: Bump s390x to ubuntu:24.04 (MarcoFalke)
20e6e8d Change Luke Dashjr seed to dashjr-list-of-p2p-nodes.us (Luke Dashjr)
a6862c5 depends: fix mingw-w64 Qt DEBUG=1 build (fanquake)

Pull request description:

  Backports:
  * #29691
  * #29747
  * #29776
  * #29853
  * #29856
  * #29859
  * #29869
  * #29870
  * #29886
  * #29892
  * #29934
  * #29985

ACKs for top commit:
  willcl-ark:
    reACK bd5860b
  stickies-v:
    re-ACK bd5860b
  TheCharlatan:
    ACK bd5860b

Tree-SHA512: a1a40de70cf52b5fc01d9dcc772421751a18c6a48a726c4c05c0371c585a53a27902e17daed9e0d721ab7763c94bb32de05c146bd6bc73fd558edd08b31e8547
Fabcien added a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 6, 2024
@Fabcien
test: Fix failing univalue float test
6df79e2 
Summary:
```
Currently the test may fail for some compilers, because 1e-8 may not be possible to represent exactly/consistently.
```

Backport of [[bitcoin/bitcoin#29892 | core#29892]].

Depends on D16296.

Test Plan:
  ninja check check-univalue

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D16297
@bitcoin bitcoin locked and limited conversation to collaborators Apr 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@stickies-v stickies-v stickies-v approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @DrahtBot @laanwj @fanquake @stickies-v