zstd
zstd copied to clipboard
facebook
Meson test fixups
- don't make running the valgrind tests mandatory
- mark known-broken test on Windows, to work around #3119
After manually installing these fixes into the Meson WrapDB, I finally got both Windows and macOS (new additions to our test matrix) to pass CI: https://github.com/mesonbuild/wrapdb/runs/6089540648?check_suite_focus=true
Is there anything else I should do about this?
As far as discussion over the second commit goes, it seems the attempt to actually fix the issue isn't getting anywhere. I think it's fine to add a "known broken" label to the testsuite.
I don't remember all the details, it seems that :
- On Windows + MinGW, and likely Windows + Visual,
zstreamtest --newpifails sometimes. But it's not reliably reproducible, meaning it fails at different places between consecutive runs on the same exact test. - This seems to points at a race condition in the multi-threading code. And it's possibly related to the shim
pthreadtranslation layer for Windows, since there is no such issue when usingpthreaddirectly, as in Windows + Cygwin combination for example, which works fine. - The proposal in this PR is to hide the issue by disabling this test, or making its result non-blocking, when run on Windows.
I think it's fair to say that I would largely prefer to have the issue fixed. Problem is, the fix is not yet found, and it's a hard problem because the issue is very difficult to observe. And a previous fix attempt, in #3121, failed to reach the objective.
Given a choice, I would still prefer to fix the issue. Problem is, finding the time to look into it is difficult, our team is tied by other priorities. We can still make this item a priority objective for the next release, but it doesn't tell when someone will take care of it. It just imply that it's probably going to happen this year.
The proposal in this PR is to hide the issue by disabling this test, or making its result non-blocking, when run on Windows.
To be clear, the proposal is to make it be non-blocking, but visibly reported:
passing tests, yay : XXX
failing tests, phew: 0
known broken? :/ : 1
This is something that the Test Anything protocol calls "todo" tests: https://testanything.org/tap-specification.html#todo-tests
These tests represent a feature to be implemented or a bug to be fixed and act as something of an executable “things to do” list.
Automake's "Generalities about testing" mentions:
It’s not uncommon, especially during early development stages, that some tests fail for known reasons, and that the developer doesn’t want to tackle these failures immediately (this is especially true when the failing tests deal with corner cases).
It's not uncommon to make a test failure be non-blocking when you've temporarily exhausted your efforts at solving it (and you have a tracking issue for it, and the testsuite flags it as a FIXME). If the test failure doesn't indicate a critical release blocker, then you want the testsuite status to be useful -- i.e. it should be non-blocking by default, and only raise critical errors if new or unknown issues are introduced.
OK, this will be discussed with the team, so that we can provide a feedback on this PR in a reasonable delay.
Follow up : @terrelln is going to have a look at this topic. It requires a working Windows machine, with development tools, so it's gonna take a little time, but hopefully nothing too long. The hope is that a fix can be found quickly, on the ground that the problem can be reproduced pretty easily. But if not, we'll revisit this proposal of having a non-blocking test while waiting for a (longer term) fix.
@terrelln did you ever have a chance to investigate this?
I'll take a look at this today / tomorrow.
An update: I've reproduced the problem and found the root cause. There's indeed a bug in the windows threading translation layer. Will put up a PR with a fix soon.
Fantastic! Thank you for your investigation. Looking forward to being able to pass tests on the next zstd release. :)
Added CI improvements on top. This also includes #3368 which is needed to configure the contrib directory so that CI runs, although that PR is much more easily merged.
Looks good, although a bit unstable as the windows test might actually pass sometimes. Merging so we can merge the other PR that solves the bug and would fix the CI to expect to pass.
Oops, yeah, I think I grabbed a hash for the msvc-dev-cmd action, from the wrong branch.
Honestly, I really wish it was possible to target semantically meaningful versions of an action, like I originally did, and specify a checksum to verify, instead of abusing commit hashes. But GitHub doesn't support that. :(