Add should_error kwarg to test()

[gerioldman]

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.

[eli-schwartz]

should_fail is equivalent to TAP's "todo" status. How does should_error map to TAP?

[gerioldman]

@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

[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.

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]

@eli-schwartz Is the current commit what you had in mind?

[eli-schwartz]

The general thrust of this commit is indeed matching the high-level vision I imagined.

Some implementation notes below.

[eli-schwartz]

Use FeatureDeprecated.single_use here, so we can gate the deprecation warning based on the project's minimum supported meson_version.

[eli-schwartz]

Actually hold on, this is already covered in the KwargInfo handling via

deprecated='1.6.0', deprecated_message='Use expected_fail instead of should_fail'),

[gerioldman]

I removed it, I thought KwargInfo handling didn't give exact location at first

[eli-schwartz]

I would set a new expected_fail = kwargs['should_fail'] variable here to avoid mutating the inputs of the current function.

/cc @dcbaker

[gerioldman]

That's probably a good idea, done :)

[eli-schwartz]

Given below we handle the else case as such:

else:
    self.res = TestResult.FAIL if bool(self.returncode) else TestResult.OK

Seems like a TestResult.OK is emitted for either a returncode of 0, or a returncode of an overridden success_returncode. Is this intentional?

[gerioldman]

That's an oversight on my part, corrected it and I added a test for this