Add should_error kwarg to test() #13559

gerioldman · 2024-08-18T14:02:51Z

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 · 2024-08-20T01:25:10Z

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

gerioldman · 2024-08-20T04:27:32Z

@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 · 2024-08-20T04:51:26Z

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 · 2024-09-04T15:48:10Z

@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 · 2024-09-04T15:58:57Z

mesonbuild/interpreter/interpreter.py

+        if kwargs['should_fail'] is not None and kwargs['expected_fail'] is not None:
+            raise InvalidArguments('Tried to use both \'should_fail\' and \'expected_fail\'')
+        elif kwargs['should_fail'] is not None:
+            mlog.deprecation('Use expected_fail instead of should_fail.', location=node)


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

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'),

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

eli-schwartz · 2024-09-04T16:00:39Z

mesonbuild/interpreter/interpreter.py

+            kwargs['expected_fail'] = kwargs['should_fail']
+        elif kwargs['expected_fail'] is None:
+            kwargs['expected_fail'] = False


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

/cc @dcbaker

That's probably a good idea, done :)

eli-schwartz · 2024-09-04T16:05:25Z

mesonbuild/mtest.py

@@ -1038,6 +1039,8 @@ class TestRunExitCode(TestRun):
    def complete(self) -> None:
        if self.res != TestResult.RUNNING:
            pass
+        elif self.returncode == self.success_returncode:
+            self.res = TestResult.OK


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?

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

bonzini · 2025-02-04T19:07:06Z

docs/markdown/snippets/expected_error.md

@@ -0,0 +1,3 @@
+## Deprecate `should_fail` and rename it to `expected_error`, also introduce `success_returncode`
+
+In 1.8.0 `should_fail` has been renamed to `expected_error`. Before 1.8.0, there was no way to positively test a command/binary returning error/non-zero return code, `success_returncode` has been introduced to achieve this.


expected_error should be expected_fail here. Also success_exitcode might be more consistent with protocol: 'exitcode'.

Maybe expected_exitcode?

bonzini · 2025-02-14T22:42:07Z

Fixes #10479

…itcode for exitcode tests

gerioldman requested a review from jpakkane as a code owner August 18, 2024 14:02

gerioldman force-pushed the expected_error branch from f13224d to d63ea45 Compare August 18, 2024 14:04

gerioldman force-pushed the expected_error branch 5 times, most recently from 6b62894 to 93a175b Compare September 4, 2024 14:35

eli-schwartz requested changes Sep 4, 2024

View reviewed changes

gerioldman force-pushed the expected_error branch 2 times, most recently from 884c54f to 0ede368 Compare September 4, 2024 19:02

gerioldman requested a review from eli-schwartz October 14, 2024 20:47

gerioldman force-pushed the expected_error branch 2 times, most recently from 39082ff to b591ad3 Compare February 4, 2025 18:45

bonzini mentioned this pull request Feb 12, 2025

Add "invert_result" to test() option #14244

Closed

bonzini added this to the 1.8 milestone Feb 12, 2025

bonzini added the test targets label Feb 12, 2025

bonzini suggested changes Feb 12, 2025

View reviewed changes

gerioldman force-pushed the expected_error branch 2 times, most recently from 32a5db7 to eed858b Compare February 22, 2025 16:26

gerioldman force-pushed the expected_error branch from eed858b to 103f5e3 Compare March 1, 2025 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add should_error kwarg to test() #13559

Add should_error kwarg to test() #13559

gerioldman commented Aug 18, 2024

eli-schwartz commented Aug 20, 2024

gerioldman commented Aug 20, 2024 •

edited

Loading

eli-schwartz commented Aug 20, 2024

gerioldman commented Sep 4, 2024

eli-schwartz left a comment

eli-schwartz Sep 4, 2024

eli-schwartz Sep 4, 2024

gerioldman Sep 4, 2024

eli-schwartz Sep 4, 2024

gerioldman Sep 4, 2024

eli-schwartz Sep 4, 2024

gerioldman Sep 4, 2024

bonzini Feb 4, 2025

gerioldman Feb 19, 2025

bonzini commented Feb 14, 2025

		@@ -0,0 +1,3 @@
		## Deprecate `should_fail` and rename it to `expected_error`, also introduce `success_returncode`

		In 1.8.0 `should_fail` has been renamed to `expected_error`. Before 1.8.0, there was no way to positively test a command/binary returning error/non-zero return code, `success_returncode` has been introduced to achieve this.

Add should_error kwarg to test() #13559

Are you sure you want to change the base?

Add should_error kwarg to test() #13559

Conversation

gerioldman commented Aug 18, 2024

eli-schwartz commented Aug 20, 2024

gerioldman commented Aug 20, 2024 • edited Loading

eli-schwartz commented Aug 20, 2024

gerioldman commented Sep 4, 2024

eli-schwartz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonzini commented Feb 14, 2025

gerioldman commented Aug 20, 2024 •

edited

Loading