mesonbuild
Add should_error kwarg to test()
This PR adds a new kwarg named 'should_error' to the test call that is similar to should_fail. It behaves the same, the only difference is that it expects error instead of fail.
The use case is that I am running some some tests where I purposefully inject failures that end up as 'Bail out!' with TAP, so the test result is expected to be error.
should_fail is equivalent to TAP's "todo" status. How does should_error map to TAP?
@eli-schwartz
If should_fail maps to "todo", then I guess I've been abusing how should_fail works. I think should_error would map to an expected 'Bail out!', which is a test ended due to a fatal error. I guess it can be interpreted as "todo" as well.
My use case is that I have tests I intentionally inject with failures/errors and I expect the test to fail to see if the failure is caught. In which case, "failing" actually means pass, in the sense that Meson returns 0, not failing the CI job.
should_fail does this for failures, so Meson returns 0, but there is no way to do this currently for errors.
Example CI run with this PR's content: https://gitlab.com/gergo.krisztian/opentestloader/-/jobs/7608089075
If should_fail maps to "todo", then I guess I've been abusing how should_fail works. I think should_error would map to an expected 'Bail out!', which is a test ended due to a fatal error. I guess it can be interpreted as "todo" as well.
Semantically, a should_fail results in meson logging the test result as an "Expected Fail:" instead of an "Ok:", and if the test starts passing it is logged as an "Unexpected Pass:", again instead of an actual "Ok:".
The reason there are 3 different statuses for this is because the "Expected Fail" list is a list of things you should be reminded to take care of at some point and "Unexpected Pass:" is stuff you have fixed and need to remove the todo note so that you start checking it again.
...
On a practical level, "Expected Fail" doesn't result in a test results failure and "Unexpected Pass" does, which means setting should_fail effectively inverts the exit code.
I am discontented with the confusion this causes. I would like to see should_fail deprecated, have it renamed to expected_fail, and have a brand new success_returncode implemented for people who want to positively test (result: "Ok") that a command binary returns errors when run.
Also, probably, expected_fail should only be permitted when the test itself is run via protocol: 'exitcode'. e.g. users of TAP should be relying on emitting "todo" markers to flag this, and users who simply want to purposefully inject test failures would be better off with asserting that a return code other than 0 should be considered the "Ok" condition.
@gerioldman can you please rebase?
I did, I will check the CI failures tomorrow.
It's just a stray should_fail in test\ cases/rust/9\ unit\ tests/meson.build.
Should specifying both expected_errorcode and should/expected_fail at the same time be an error?
Combining both is quite confusing at least to me. The former is "I need this exact thing to happen" whereas the latter is "I need the opposite of this exact thing to happen". The combination of those is "the error code must be anything except 42". Is that ever meaningful?
Based on the discussions on this PR, should/expected_fail is used for saying: "I know this test is failing, Meson should have exit code 0 if this test fails, and say it is an expected failure, not a pass.", while expected_exitcode is used for specifying what exitcode the accept for a test pass. So the combination would be the very rare situation of: "I have a test that expects 15 as exitcode, but returns 1 right now, and it is a expected fail."
Asking again:
Should specifying both expected_errorcode and should/expected_fail at the same time be an error?
Should specifying both expected_errorcode and should/expected_fail at the same time be an error?
No, for example a test could have expected_errorcode of 1 but it currently returns 0; then you make it expected_fail: true so that it to shows as an XFAIL rather than a PASS.
In other words expected_errorcode is about the desired behavior of the program, while expected_fail records known bugs.
@gerioldman apologies really but I noticed one more change needed (IMO). The handling of error codes 77 and 99 is a bit messy, they're not usable anymore if expected_exitcode is provided. I can see why you did it that way, but skipping tests can still be valuable and I am afraid that people will lose that by using expected_exitcode: 0 innocently.
My suggestion is
elif self.returncode == (self.expected_exitcode or 0):
self.res = TestResult.OK
elif self.returncode == GNU_SKIP_RETURNCODE:
...
else:
self.res = TestResult.FAIL
I should have noticed this before, sorry.
It doesn't use the same logic as the version with the "match" statement (b3af6e9) and I am not sure if it's intentional. Is there a reason why you didn't just rewrite the match statement as a chain of "if"s?
Sorry for the late reply, I had some other priorities. I don't exactly remember why I went with match-case, I can switch it back to if's. As for why the logic is different, the snippet you shared would make a 0 returncode pass as OK if expected_exitcode was specified. I think that's incorrect.