ci: Skip broken Wine64 tests by default

maflcko commented at 4:39 pm on November 13, 2024: member

I don’t think the unit tests run in Wine after the Windows cross-compilation have ever shown a true positive since the MSVC task was added. However, they are a source of frequent false-positives.

Thus, disable them by default for now. Anyone can still enable them by setting RUN_UNIT_TESTS=true.

A follow-up could run them on real Windows, see #31176.

Conceptually there are many other nightly tasks, which rarely find issues and are not run by default, like the valgrind or s390x tasks. So putting the Wine unit tests in the same bucket should be fine.

DrahtBot commented at 4:39 pm on November 13, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31284.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, willcl-ark

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31176 (ci: Test cross-built Windows executables on Windows natively by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Nov 13, 2024

ci: Skip broken Wine64 tests by default fa5e706459

maflcko force-pushed on Nov 13, 2024

maflcko commented at 6:12 pm on November 13, 2024: member

Checked that RUN_UNIT_TESTS=true MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh still works.

maflcko requested review from hebasto on Nov 21, 2024

maflcko commented at 10:32 am on November 22, 2024: member

The false positives keep coming in, so it would be good to make progress here:

https://cirrus-ci.com/task/4830737755537408?logs=ci#L2355

hebasto commented at 10:36 am on November 22, 2024: member

I’m a bit hesitant about this change.

Conceptually there are many other nightly tasks, which rarely find issues and are not run by default, like the valgrind or s390x tasks. So putting the Wine unit tests in the same bucket should be fine.

I disagree. Conceptually, this is a different case because the cross-compiled test_bitcoin.exe is part of release bundle for Windows. And leaving the repo without a CI job to run it seems like a drawback.

A follow-up could run them on real Windows, but this comes with its own downsides, see #31176.

What downsides?

maflcko commented at 11:09 am on November 22, 2024: member

the cross-compiled test_bitcoin.exe is part of release bundle for Windows. And leaving the repo without a CI job to run it seems like a drawback.

How is this different from the whole macos-cross release, or even the real binary bitcoind.exe, which is never tested? Moreover, if you think that running the windows unit tests on Wine is useful, it would be good to give at least one example where it caught an issue that was found by none of the other tasks. (For example, they can’t even detect a MAX_PATH violation in the unit tests, which was found by the MSVC task, but not the Wine one). Finally, the task will still be run, and issues will still be reported (if they ever happen and are not a false-positive). However, if the task only ever produces false-positives (and distributes them over this repo), there is a real cost to it, and if there are any upsides to having this tasks, they will also need to be real. (If no such real example can be found, I think it is also fine to wait for one to happen in the future, and then re-add the task, if it was meaningful enough)

A follow-up could run them on real Windows, but this comes with its own downsides, see #31176.

What downsides?

See the individual comments. Given that there are two threads, I think it would be good to keep the discussion of each one focussed. Thus, I’ve removed “downsides” from this sentence in the pull description.

hebasto approved

hebasto commented at 11:36 am on November 22, 2024: member

ACK fa5e7064597fc51366f082e3d07a4591e576db38, to avoid false-positives.

maflcko commented at 12:55 pm on November 25, 2024: member

The false positives keep coming in, so it would be good to make progress here:

https://cirrus-ci.com/task/5175499276681216

maflcko commented at 9:14 am on November 26, 2024: member

… https://cirrus-ci.com/task/5957633356595200

maflcko added this to the milestone 29.0 on Nov 26, 2024

willcl-ark approved

willcl-ark commented at 3:10 pm on November 28, 2024: member

ACK fa5e7064597fc51366f082e3d07a4591e576db38

I agree with disabling these tests, and then pursuing a followup e.g. #31176 to run them natively on windows. There’s no point in debugging false positives (and no? real positives).

maflcko commented at 2:28 pm on December 2, 2024: member

… https://cirrus-ci.com/task/4678171796176896?logs=ci#L2624

maflcko commented at 2:29 pm on December 2, 2024: member

Is this test-only change with two reviews rfm?

This is the most common false positive right now.

maflcko commented at 2:46 pm on December 2, 2024: member

… https://cirrus-ci.com/task/6390830670282752?logs=ci#L4462

maflcko commented at 4:32 pm on December 2, 2024: member

… https://cirrus-ci.com/task/6299029368209408?logs=ci#L4685

maflcko commented at 8:59 pm on December 2, 2024: member

… https://cirrus-ci.com/task/5540900196057088?logs=ci#L4684

hebasto commented at 1:44 pm on December 3, 2024: member

Is this test-only change with two reviews rfm?

This is the most common false positive right now.

And the replacement is ready.

Fixes #31071

Maybe drop it from the PR description?

maflcko commented at 1:54 pm on December 3, 2024: member

I don’t think the replacement has received any review/acks, so I think it still makes sense to merge this one in the meantime. I don’t think anyone benefits from the flood of false positives here.

hebasto commented at 2:27 pm on December 3, 2024: member

… so I think it still makes sense to merge this one in the meantime.

I did not mean to delay the merge of this PR.

willcl-ark commented at 2:38 pm on December 3, 2024: member

https://cirrus-ci.com/task/6357677415071744?logs=ci#L2640

maflcko commented at 5:11 pm on December 3, 2024: member

@ryanofsky Do you think this is rfm?

ryanofsky merged this on Dec 3, 2024

ryanofsky closed this on Dec 3, 2024

ryanofsky commented at 5:30 pm on December 3, 2024: contributor

@ryanofsky Do you think this is rfm?

Merged this now.

Am a little curious about the underlying issue though because in theory there doesn’t have to be binary distinction between false positives and true positives. Test failures on a platform could indicate that be a sign that tests are fragile or poorly written even if they aren’t technically buggy or reveal bugs in the code. Just clicking the cirrus links here it’s hard to tell what the nature of these failures is, what may be causing them, and whether they happen reliably or randomly.

maflcko commented at 6:16 pm on December 3, 2024: member

Just clicking the cirrus links here it’s hard to tell what the nature of these failures is, what may be causing them, and whether they happen reliably or randomly.

The nature of the failures is:

They are caused by Wine.
They happen randomly.
They are known for years (A random mention of them from 2017 is https://github.com/bitcoin/bitcoin/blame/8e02b480591008c457cc841050f6f755ff54fcba/test/util/test_runner.py#L162). If you use a search engine you may find more or earlier mentions inside or outside of this repo.
The fragility is likely Wine, which is not a supported platform in any case.

Simply going directly with Windows (a supported platform) should be done as a follow-up, see https://github.com/bitcoin/bitcoin/pull/31176

maflcko deleted the branch on Dec 3, 2024

ci: Skip broken Wine64 tests by default #31284

Code Coverage & Benchmarks

Reviews

Conflicts