Meson test fixups #3120

eli-schwartz · 2022-04-27T01:00:08Z

don't make running the valgrind tests mandatory
mark known-broken test on Windows, to work around "./zstreamtest --newapi" test case fails on Windows #3119

eli-schwartz · 2022-04-27T01:02:33Z

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

Cyan4973 · 2022-04-27T17:54:59Z

build/meson/tests/meson.build

@@ -183,6 +185,8 @@ test('test-zstream-1',
 test('test-zstream-3',
  zstreamtest,
  args: ['--newapi', '-t1', ZSTREAM_TESTTIME] + FUZZER_FLAGS,
+  # --newapi dies on Windows with "exit status 3221225477 or signal 3221225349 SIGinvalid"
+  should_fail: host_machine_os == os_windows,


what if the issue is fixed, and zstreamtest --newapi now completes successfully ?
would it be considered a test failure due to should_fail ?

This PR changes its status to EXPECTEDFAIL, which is a success status. When the bug is fixed, it will start producing an UNEXPECTEDPASS status, which is considered a failure and causes the overall test results to be negative.

This PR changes its status to EXPECTEDFAIL, which is a success status. When the bug is fixed, it will start producing an UNEXPECTEDPASS status, which is considered a failure and causes the overall test results to be negative.

Better fix the issue then

Yep, once that other PR is merged I will re-check this and drop the obsolete commit.

eli-schwartz · 2022-10-13T05:35:45Z

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.

Cyan4973 · 2022-10-13T06:01:05Z

I don't remember all the details,
it seems that :

On Windows + MinGW, and likely Windows + Visual, zstreamtest --newpi fails 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 pthread translation layer for Windows, since there is no such issue when using pthread directly, 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.

eli-schwartz · 2022-10-13T06:16:47Z

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.

Cyan4973 · 2022-10-13T06:26:54Z

OK, this will be discussed with the team, so that we can provide a feedback on this PR in a reasonable delay.

eli-schwartz · 2022-10-13T07:02:53Z

Alright, thanks for looking into it. :)

Cyan4973 · 2022-10-13T20:46:12Z

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.

eli-schwartz · 2022-12-01T04:28:41Z

Any update on this? :)

eli-schwartz · 2022-12-14T21:35:46Z

@terrelln did you ever have a chance to investigate this?

yoniko · 2022-12-14T21:47:15Z

@terrelln did you ever have a chance to investigate this?

I'll take a look at this today / tomorrow.

yoniko · 2022-12-15T23:24:36Z

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.

eli-schwartz · 2022-12-15T23:57:45Z

Fantastic! Thank you for your investigation. Looking forward to being able to pass tests on the next zstd release. :)

1. If threads are resized the threads' `ZSTD_pthread_t` might move while the worker still holds a pointer into it (see more details in facebook#3120). 2. The join operation was waiting for a thread and then return its `thread.arg` as a return value, but since the `ZSTD_pthread_t thread` was passed by value it would have a stale `arg` that wouldn't match the thread's actual return value. This fix changes the `ZSTD_pthread_join` API and removes support for returning a value. This means that we are diverging from the `pthread_join` API and this is no longer just an alias. In the future, if needed, we could return a Windows thread's return value using `GetExitCodeThread`, but as this path wouldn't be excised in any case, it's preferable to not add it right now.

eli-schwartz · 2022-12-16T04:04:08Z

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.

yoniko

Looks good, my only feedback is that I think it'd be better to pin the third-party action to a specific commit.
After making the change I think it'd be a good idea to merge this and then merge a PR that fixes the issue and removes the expected failure for windows.

.github/workflows/dev-short-tests.yml

1. If threads are resized the threads' `ZSTD_pthread_t` might move while the worker still holds a pointer into it (see more details in facebook#3120). 2. The join operation was waiting for a thread and then return its `thread.arg` as a return value, but since the `ZSTD_pthread_t thread` was passed by value it would have a stale `arg` that wouldn't match the thread's actual return value. This fix changes the `ZSTD_pthread_join` API and removes support for returning a value. This means that we are diverging from the `pthread_join` API and this is no longer just an alias. In the future, if needed, we could return a Windows thread's return value using `GetExitCodeThread`, but as this path wouldn't be excised in any case, it's preferable to not add it right now.

It's entirely possible some people don't have valgrind installed, but still want to run the tests. If they don't have it installed, then they probably don't intend to run those precise test targets anyway. Also, this solves an error when running the tests in an automated environment. The valgrind tests have a hard dependency on behavior such as `./zstd` erroring out with the message "stdin is a console, aborting" which does not work if the automated environment doesn't have a console. As a rough heuristic, automated environments lacking a console will *probably* also not have valgrind, so avoiding that test definition neatly sidesteps the issue. Also, valgrind is not easily installable on macOS, at least homebrew says it isn't available there. This makes it needlessly hard to enable the testsuite on macOS.

playTests.sh has an option to run really slow tests. This is enabled by default in Meson, but what we really want is to do like the Makefile, and run the fast ones by default, but with an option to run the slow ones instead.

Travis is no longer run, but this wasn't ported to something else.

There are a couple of oddities here. We don't attempt to build e.g. contrib, because that doesn't seem to work at the moment. Also notice that each command is its own step. This happens because github actions runs in powershell, which doesn't seem to let you abort on the first failure.

yoniko · 2022-12-17T02:28:35Z

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.

1. If threads are resized the threads' `ZSTD_pthread_t` might move while the worker still holds a pointer into it (see more details in facebook#3120). 2. The join operation was waiting for a thread and then return its `thread.arg` as a return value, but since the `ZSTD_pthread_t thread` was passed by value it would have a stale `arg` that wouldn't match the thread's actual return value. This fix changes the `ZSTD_pthread_join` API and removes support for returning a value. This means that we are diverging from the `pthread_join` API and this is no longer just an alias. In the future, if needed, we could return a Windows thread's return value using `GetExitCodeThread`, but as this path wouldn't be excised in any case, it's preferable to not add it right now.

eli-schwartz · 2022-12-17T23:24:26Z

Oops, yeah, I think I grabbed a hash for the msvc-dev-cmd action, from the wrong branch.

eli-schwartz · 2022-12-17T23:26:13Z

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. :(

facebook-github-bot added the CLA Signed label Apr 27, 2022

Cyan4973 reviewed Apr 27, 2022

View reviewed changes

eli-schwartz mentioned this pull request Dec 1, 2022

CI seems to have a dependence to https://mirror.download.kiwix.org/dev/zstd-1.4.5-wrap.zip openzim/libzim#663

Closed

Cyan4973 assigned yoniko Dec 14, 2022

eli-schwartz mentioned this pull request Dec 14, 2022

Meson build fails on windows #2358

Closed

yoniko mentioned this pull request Dec 16, 2022

Windows MT layer bug fixes #3364

Merged

eli-schwartz force-pushed the meson-fixup branch from 68ab1b2 to 887d06a Compare December 16, 2022 04:02

yoniko requested changes Dec 16, 2022

View reviewed changes

.github/workflows/dev-short-tests.yml Outdated Show resolved Hide resolved

eli-schwartz added 4 commits December 16, 2022 14:27

meson: add Linux CI

7f29c18

Travis is no longer run, but this wasn't ported to something else.

meson: mark a known test failure on Windows

6747ba4

eli-schwartz force-pushed the meson-fixup branch from 887d06a to 6747ba4 Compare December 16, 2022 19:36

yoniko merged commit ce61cb8 into facebook:dev Dec 17, 2022

eli-schwartz deleted the meson-fixup branch December 17, 2022 23:23

Cyan4973 mentioned this pull request Feb 9, 2023

release v1.5.4 #3487

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Meson test fixups #3120

Meson test fixups #3120

eli-schwartz commented Apr 27, 2022

eli-schwartz commented Apr 27, 2022

Cyan4973 Apr 27, 2022

eli-schwartz Apr 27, 2022

Cyan4973 Apr 28, 2022

eli-schwartz Apr 28, 2022

eli-schwartz commented Oct 13, 2022

Cyan4973 commented Oct 13, 2022

eli-schwartz commented Oct 13, 2022

Cyan4973 commented Oct 13, 2022

eli-schwartz commented Oct 13, 2022

Cyan4973 commented Oct 13, 2022

eli-schwartz commented Dec 1, 2022

eli-schwartz commented Dec 14, 2022

yoniko commented Dec 14, 2022

yoniko commented Dec 15, 2022

eli-schwartz commented Dec 15, 2022

eli-schwartz commented Dec 16, 2022

yoniko left a comment

yoniko commented Dec 17, 2022

eli-schwartz commented Dec 17, 2022

eli-schwartz commented Dec 17, 2022

Meson test fixups #3120

Meson test fixups #3120

Conversation

eli-schwartz commented Apr 27, 2022

eli-schwartz commented Apr 27, 2022

Cyan4973 Apr 27, 2022

Choose a reason for hiding this comment

eli-schwartz Apr 27, 2022

Choose a reason for hiding this comment

Cyan4973 Apr 28, 2022

Choose a reason for hiding this comment

eli-schwartz Apr 28, 2022

Choose a reason for hiding this comment

eli-schwartz commented Oct 13, 2022

Cyan4973 commented Oct 13, 2022

eli-schwartz commented Oct 13, 2022

Cyan4973 commented Oct 13, 2022

eli-schwartz commented Oct 13, 2022

Cyan4973 commented Oct 13, 2022

eli-schwartz commented Dec 1, 2022

eli-schwartz commented Dec 14, 2022

yoniko commented Dec 14, 2022

yoniko commented Dec 15, 2022

eli-schwartz commented Dec 15, 2022

eli-schwartz commented Dec 16, 2022

yoniko left a comment

Choose a reason for hiding this comment

yoniko commented Dec 17, 2022

eli-schwartz commented Dec 17, 2022

eli-schwartz commented Dec 17, 2022