ci: Catch assertion error message into logs for test_bitcoin.exe #24363

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:220216-assert changing 1 files +7 −1

hebasto commented at 10:46 PM on February 16, 2022: member

Fixes bitcoin/bitcoin#24261.

A demo CI build with an added assert(false): https://cirrus-ci.com/build/6165030604374016
ci: Catch assertion error message into logs for test_bitcoin.exe c72462eecd
hebasto added the label Windows on Feb 16, 2022
hebasto added the label Tests on Feb 16, 2022
hebasto marked this as ready for review on Feb 16, 2022
in ci/test/wrap-wine.sh:16 in c72462eecd
```
  12 | @@ -13,7 +13,13 @@ for b_name in {"${BASE_OUTDIR}/bin"/*,src/secp256k1/*tests,src/minisketch/test{,
  13 |          echo "Wrap $b ..."
  14 |          mv "$b" "${b}_orig"
  15 |          echo '#!/usr/bin/env bash' > "$b"
  16 | -        echo "( wine \"${b}_orig\" \"\$@\" ) || ( sleep 1 && wine \"${b}_orig\" \"\$@\" )" >> "$b"
  17 | +        if test "$(basename "$b")" = "test_bitcoin.exe"; then
```
maflcko commented at 6:25 AM on February 17, 2022:

any need for this condition?

hebasto commented at 6:33 AM on February 17, 2022:

Yes. Without it python tests in test/util got crazy. Did not investigate further though.

ryanofsky commented at 4:21 PM on February 18, 2022:

In commit "ci: Catch assertion error message into logs for test_bitcoin.exe" (c72462eecdecf606f5bc5f7f90b7f732f404607b)

One problem with using script here is that it combines stdout and stderr into the same stream. This works for how we call test_bitcoin, but not surprising it would be broken for other binaries.

Probably it would be more ideal to fix this by using script in src/Makefile.test.include instead of the less reliable > $<.log 2>&1 || (cat $<.log && false) which could cause buffered output to be lost on other platforms, not just windows. If script were used there, the changes here could be reverted.

hebasto commented at 4:15 PM on February 21, 2022:

Probably it would be more ideal to fix this by using script in src/Makefile.test.include instead of the less reliable > $<.log 2>&1 || (cat $<.log && false) which could cause buffered output to be lost on other platforms, not just windows. If script were used there, the changes here could be reverted.

Great idea, but BSD version of script on macOS is incompatible with Linux version, e.g., it lacks the --return option.

laanwj commented at 7:49 AM on April 14, 2022:

Great idea, but BSD version of script on macOS is incompatible with Linux version, e.g., it lacks the --return option.

Candidate for python port? :sweat_smile:

in ci/test/wrap-wine.sh:18 in c72462eecd

  12 | @@ -13,7 +13,13 @@ for b_name in {"${BASE_OUTDIR}/bin"/*,src/secp256k1/*tests,src/minisketch/test{,
  13 |          echo "Wrap $b ..."
  14 |          mv "$b" "${b}_orig"
  15 |          echo '#!/usr/bin/env bash' > "$b"
  16 | -        echo "( wine \"${b}_orig\" \"\$@\" ) || ( sleep 1 && wine \"${b}_orig\" \"\$@\" )" >> "$b"
  17 | +        if test "$(basename "$b")" = "test_bitcoin.exe"; then
  18 | +          # Using `script` allows to catch assertion error messages into logs.
  19 | +          echo "COMMAND=\"wine ${b}_orig \$@\"" >> "$b"

ryanofsky commented at 4:45 PM on February 18, 2022:

In commit "ci: Catch assertion error message into logs for test_bitcoin.exe" (c72462eecdecf606f5bc5f7f90b7f732f404607b)

If arguments contain whitespace or shell characters they won't be escaped here correctly. It's probably not important to fix this, but you could probably do with with

echo "COMMAND=\"wine ${b}_orig\$(printf \" %q\" \"\$@\")\"" >> "$b"

But still better would be to follow earlier comment and revert this

ryanofsky approved
ryanofsky commented at 4:47 PM on February 18, 2022: contributor

Code review ACK c72462eecdecf606f5bc5f7f90b7f732f404607b. This fix isn't the most ideal (see suggested alternative below) but it fixes a real problem and is definitely an improvement
DrahtBot commented at 1:03 PM on September 23, 2022: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK ryanofsky

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



Conflicts

No conflicts as of last run.
fanquake commented at 3:09 PM on January 31, 2023: member

What's the status of this? I can't see a reason to not use the better fixes as outlined in the comments.
hebasto commented at 10:37 AM on February 3, 2023: member

Closing as no longer relevant.
hebasto closed this on Feb 3, 2023
bitcoin locked this on Feb 3, 2024

Type	Reviewers
ACK	ryanofsky

Contributors

Labels

ci: Catch assertion error message into logs for test_bitcoin.exe #24363

Reviews

Conflicts