[PATCH] tests: Record negative tests as pass instead of expected-fail
meson's "test()" function provides a "should_fail: bool" argument that checks for a command to exit with a non-zero exit code instead of the usual zero exit code to signal success. If the program under test does so, it is recorded as "EXPECTEDFAIL" instead of "OK". While there is an argument to be made that the program under test failed as expected, the test in itself is successful and should be recorded as such. Before: $ meson test ... 151/300 libvirt:bin / libvirtd fail with missing config EXPECTEDFAIL 0.03s exit status 1 ... Ok: 299 Expected Fail: 1 Fail: 0 Unexpected Pass: 0 Skipped: 0 Timeout: 0 After: $ meson test ... 151/300 libvirt:bin / libvirtd fail with missing config OK 0.03s ... Ok: 300 Expected Fail: 0 Fail: 0 Unexpected Pass: 0 Skipped: 0 Timeout: 0 Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/expected-fail | 13 +++++++++++++ tests/meson.build | 5 ++--- 2 files changed, 15 insertions(+), 3 deletions(-) create mode 100755 tests/expected-fail diff --git a/tests/expected-fail b/tests/expected-fail new file mode 100755 index 0000000000..85738c95dc --- /dev/null +++ b/tests/expected-fail @@ -0,0 +1,13 @@ +#!/bin/sh + +# meson's "test()" function provides a "should_fail: bool" argument that +# checks for a command to exit with a non-zero exit code instead of the usual +# zero exit code to signal success. If the program under test does so, it is +# recorded as "EXPECTEDFAIL" instead of "OK". While there is an argument to be +# made that the program under test failed as expected, the test in itself is +# successful and should be recorded as such. + +if "$@" +then + exit 1 +fi diff --git a/tests/meson.build b/tests/meson.build index 0d76d37959..0ebcdc1496 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -725,9 +725,8 @@ endif if conf.has('WITH_LIBVIRTD') test('libvirtd fail with missing config', - libvirtd_prog, - args: [ '--config=no-such-conf', '--timeout=5' ], - should_fail: true, + find_program('expected-fail'), + args: [ libvirtd_prog, '--config=no-such-conf', '--timeout=5' ], suite: 'bin', ) -- 2.48.1
On Mon, Feb 10, 2025 at 11:25:55 +0100, Tim Wiederhake wrote:
meson's "test()" function provides a "should_fail: bool" argument that checks for a command to exit with a non-zero exit code instead of the usual zero exit code to signal success. If the program under test does so, it is recorded as "EXPECTEDFAIL" instead of "OK". While there is an argument to be made that the program under test failed as expected, the test in itself is successful and should be recorded as such.
Could you please elaborate what the problem is? The wording of the
Before: $ meson test ... 151/300 libvirt:bin / libvirtd fail with missing config EXPECTEDFAIL 0.03s exit status 1
... EXPECTEDFAIL case?
... Ok: 299 Expected Fail: 1 Fail: 0 Unexpected Pass: 0 Skipped: 0 Timeout: 0
After: $ meson test ... 151/300 libvirt:bin / libvirtd fail with missing config OK 0.03s ... Ok: 300 Expected Fail: 0 Fail: 0 Unexpected Pass: 0 Skipped: 0 Timeout: 0
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/expected-fail | 13 +++++++++++++ tests/meson.build | 5 ++--- 2 files changed, 15 insertions(+), 3 deletions(-) create mode 100755 tests/expected-fail
diff --git a/tests/expected-fail b/tests/expected-fail new file mode 100755 index 0000000000..85738c95dc --- /dev/null +++ b/tests/expected-fail @@ -0,0 +1,13 @@ +#!/bin/sh + +# meson's "test()" function provides a "should_fail: bool" argument that +# checks for a command to exit with a non-zero exit code instead of the usual +# zero exit code to signal success. If the program under test does so, it is +# recorded as "EXPECTEDFAIL" instead of "OK". While there is an argument to be +# made that the program under test failed as expected, the test in itself is +# successful and should be recorded as such. + +if "$@" +then + exit 1 +fi
The idea was exactly to avoid having this extra script.
On Mon, Feb 10, 2025 at 11:30:24 +0100, Peter Krempa wrote:
On Mon, Feb 10, 2025 at 11:25:55 +0100, Tim Wiederhake wrote:
meson's "test()" function provides a "should_fail: bool" argument that checks for a command to exit with a non-zero exit code instead of the usual zero exit code to signal success. If the program under test does so, it is recorded as "EXPECTEDFAIL" instead of "OK". While there is an argument to be made that the program under test failed as expected, the test in itself is successful and should be recorded as such.
Could you please elaborate what the problem is? The wording of the
I've re-read this and it ineed seems that just the wording is the issue. I'm not sold that we need a script to bypass the return value just for meson to say "OK".
On Mon, 2025-02-10 at 12:44 +0100, Peter Krempa wrote:
On Mon, Feb 10, 2025 at 11:30:24 +0100, Peter Krempa wrote:
On Mon, Feb 10, 2025 at 11:25:55 +0100, Tim Wiederhake wrote:
meson's "test()" function provides a "should_fail: bool" argument that checks for a command to exit with a non-zero exit code instead of the usual zero exit code to signal success. If the program under test does so, it is recorded as "EXPECTEDFAIL" instead of "OK". While there is an argument to be made that the program under test failed as expected, the test in itself is successful and should be recorded as such.
Could you please elaborate what the problem is? The wording of the
I've re-read this and it ineed seems that just the wording is the issue.
I'm not sold that we need a script to bypass the return value just for meson to say "OK".
Expected failures or "xfail" results were used as a tool to write a test for a bug before committing the fix. You'd write the test first, mark it as "xfail" to make the issue visible without making the entire test suite fail. The commit that fixes the issue would then remove the "xfail" marker and demonstrate that it actually fixes the issue, i.e. makes the test pass. Using expected-fail is not the right tool to handle a test case that is always expected to exit with a non-zero exit code. As I said, the test is "program exits non-zero", and that test passes. There is no failure in the test, expected or otherwise. Tim
On Mon, Feb 10, 2025 at 12:58:06 +0100, Tim Wiederhake wrote:
On Mon, 2025-02-10 at 12:44 +0100, Peter Krempa wrote:
On Mon, Feb 10, 2025 at 11:30:24 +0100, Peter Krempa wrote:
On Mon, Feb 10, 2025 at 11:25:55 +0100, Tim Wiederhake wrote:
meson's "test()" function provides a "should_fail: bool" argument that checks for a command to exit with a non-zero exit code instead of the usual zero exit code to signal success. If the program under test does so, it is recorded as "EXPECTEDFAIL" instead of "OK". While there is an argument to be made that the program under test failed as expected, the test in itself is successful and should be recorded as such.
Could you please elaborate what the problem is? The wording of the
I've re-read this and it ineed seems that just the wording is the issue.
I'm not sold that we need a script to bypass the return value just for meson to say "OK".
Expected failures or "xfail" results were used as a tool to write a test for a bug before committing the fix. You'd write the test first, mark it as "xfail" to make the issue visible without making the entire test suite fail. The commit that fixes the issue would then remove the "xfail" marker and demonstrate that it actually fixes the issue, i.e. makes the test pass.
Using expected-fail is not the right tool to handle a test case that is always expected to exit with a non-zero exit code. As I said, the test is "program exits non-zero", and that test passes. There is no failure in the test, expected or otherwise.
Meson developers seem to think otherwise: https://github.com/mesonbuild/meson/pull/4466#issuecomment-435688173
This is not actually true. Using expected failures to mark bugs is an antipattern and you should not do that. Tests that should fail are for things like trying to run an executable with known invalid input. It must return an error in this case rather than silently accept it.
The documentation also simply states the fact that it's for tests returning non-zero: should_fail bool when true the test is considered passed if the executable returns a non-zero return value (i.e. reports an error) Which is why I chose to do this. Based on the above I'm still not persuaded we want this. In case you still want to go forward I'd sugest you revert e57ce7fb452 instead of adding a workaround script.
On Mon, Feb 10, 2025 at 01:57:51PM +0100, Peter Krempa wrote:
On Mon, Feb 10, 2025 at 12:58:06 +0100, Tim Wiederhake wrote:
On Mon, 2025-02-10 at 12:44 +0100, Peter Krempa wrote:
On Mon, Feb 10, 2025 at 11:30:24 +0100, Peter Krempa wrote:
On Mon, Feb 10, 2025 at 11:25:55 +0100, Tim Wiederhake wrote:
meson's "test()" function provides a "should_fail: bool" argument that checks for a command to exit with a non-zero exit code instead of the usual zero exit code to signal success. If the program under test does so, it is recorded as "EXPECTEDFAIL" instead of "OK". While there is an argument to be made that the program under test failed as expected, the test in itself is successful and should be recorded as such.
Could you please elaborate what the problem is? The wording of the
I've re-read this and it ineed seems that just the wording is the issue.
I'm not sold that we need a script to bypass the return value just for meson to say "OK".
Expected failures or "xfail" results were used as a tool to write a test for a bug before committing the fix. You'd write the test first, mark it as "xfail" to make the issue visible without making the entire test suite fail. The commit that fixes the issue would then remove the "xfail" marker and demonstrate that it actually fixes the issue, i.e. makes the test pass.
Using expected-fail is not the right tool to handle a test case that is always expected to exit with a non-zero exit code. As I said, the test is "program exits non-zero", and that test passes. There is no failure in the test, expected or otherwise.
Meson developers seem to think otherwise:
https://github.com/mesonbuild/meson/pull/4466#issuecomment-435688173
This is not actually true. Using expected failures to mark bugs is an antipattern and you should not do that. Tests that should fail are for things like trying to run an executable with known invalid input. It must return an error in this case rather than silently accept it.
The documentation also simply states the fact that it's for tests returning non-zero:
should_fail bool
when true the test is considered passed if the executable returns a non-zero return value (i.e. reports an error)
Which is why I chose to do this.
Based on the above I'm still not persuaded we want this. In case you still want to go forward I'd sugest you revert e57ce7fb452 instead of adding a workaround script.
IMHO based on the meson PR comment above, the current code is working as Meson developers intended and so we don't need todo anything. The only issue I see here is that the meson docs should be improved to better explain their intentions around usage pattersn for the 'should_fail' flag, as it is ambiguous/open to differing interpretations. They should expand the docs per that PR comment explanation. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé -
Peter Krempa -
Tim Wiederhake