Implement check-result: test check failure affects test result by default

[martinhoyer]

Trying to address #3185

Pull Request Checklist

• implement the feature
• write the documentation
• extend the test coverage
• update the specification
• modify the json schema
• mention the version
• include a release note

[martinhoyer]

Rebased to @happz's result-store-original-result #3147 and tried to use beakerlib for the first time. Feels werid to do all the assertgreps, but I've been merely following the existing examples.

Still proof of concept, but at least I finally figured that the "after-test" check were not available without the change in execute/internal.py.

[happz]

@martinhoyer if you change the base branch to the one from #3147, the diff should reduce a bit.

[happz]

respect_check feels more like an input, IIUIC it's basically interpret, but in respect to checks. So, they should enter the method through the same door, so to speak.

[martinhoyer]

I see, so you want me to create

class CheckResults(enum.Enum):
    RESPECT = 'respect'
    IGNORE = 'ignore'
...

?

I'm so lost in all the schemas, specs, et al.

[happz]

I suppose we could re-use ResultInterpret, it has the same purpose, and skip can serve as ignore.

[happz]

Would this help?

return _result.interpret_result(
    ResultInterpret(invocation.test.result) if invocation.test.result else ResultInterpret.RESPECT,
    ResultInterpret(invocation.test.check_result) if invocation.test.check_result else ResultInterpret.RESPECT
)

[happz]

I'm so lost in all the schemas, specs, et al.

It shouldn't be that messy. If I can help somehow, let me know, maybe the whole area needs more documentation.

[happz]

Yes, something like this, with one exception: I don't understand why Result.check_result exists. How checks should be interpreted is a test-level metadata key, i.e. it should come from the test, not the result. Isn't it the same story as Test.result?

[martinhoyer]

Thanks. Honestly that's a leftover from the previous attempts to make things work that I missed and kept.

[happz]

In the light of #3185 (comment), no check-result on test level, I suppose...

This means new result check-level field would reside in tmt.checks.Check class (and _RawCheck), and interpretation of check results need to match a result with corresponding check in test.check list...

[martinhoyer]

Yep

[martinhoyer]

Reworked.

[martinhoyer]

tmt.utils.GeneralError: Test check 'dmesg' was not found in check registry.

perplexing

[happz]

The field should already be ResultInterpret, as mentioned in a comment above WRT its definition. And I just noticed that Test.result isn't an enum, which is definitely incorrect, and a bad example :/

[happz]

I think we can switch from Any to ResultInterpret, if we add the from_spec() method to it, unserialize could use it. Might need also normalize callback, but I'd pay that price for getting rid of Any. With Any, no user of this field can't be sure what the value is.

[martinhoyer]

@psss I got a little entangled in the xfail logic and the # Handle individual check results part should probably be done before this.

I wonder if it will be digestible enough for the users, I mean:
xfail test fails, check with default 'respect' fails = FAIL
i.e. users would have to add xfail also to check, if expected. Could be breaking change for some?

[sbertramrh]

Thanks for doing this. I would also like to petition for the following scenario.
if the before test portion is the only part that fails for the check, the test should be self.result = ResultOutcome.WARN.

[psss]

So here we're introducing a new value skip which is currently not present in the Test.result key specification. What about keeping the consistency here as well and support pass, info, warn, error, fail in the very same way?

[happz]

Hmmm,skip is definitely one of the values of https://github.com/teemtee/tmt/blob/main/tmt/result.py#L43, and it's also allowed by schema, https://github.com/teemtee/tmt/blob/main/tmt/schemas/common.yaml#L481. If it's not supposed to be supported as a result value, we should remove it from these places as well.

[happz]

Although, I'd argue the specification should be updated instead, because skip is a perfectly valid test outcome, like pass or fail, and it makes sense that one might want to enforce it as a test outcome via result, just like one might want to enforce pass or xfail.

[happz]

And +1 to accepting all keys allowed by ResultInterpret, i.e. why allowing skip and not xfail or pass. Except custom, that one does not make much sense for a test check.

[happz]

And restraint, restraint is also useless at this point of code :)

[happz]

To be honest, I got pretty frustrated with this code. I guess I'm too dumb for mixing custom mixins, uninheritable classes, specifications, schemas, et al. I'm sure there are good reasons for all of this, but idk, somehow I think doing a relatively small change should be easier.

That's sad to hear. I believe that while it might be a relatively small change, it's not a small change given the interaction of test and test check results. Schema, specification, the fact that enum inheritance is limited, well, that's a fact we can't bypass.

Anyway, should I create some BaseResultInterpretMixin class w/ SerializableContainer?

I for one don't think so. Where do you think it would be needed?

I tried to went back to using 'ignore', but now we're mixing two things together, results and how to interpret it. :(

We can get on a call and try to get through the parts that cause you trouble, if you want to. Mixing the result and its interpretation is definitely a sign of a wrong turn somewhere.

[happz]

Yeah, check_result = CheckResultInterpret(check.result.value) is definitely not the right move, check.result.value is the outcome, the interpretation must come from the check, not from its result. That's also true for test, and you see how test result interpretation gets in, via invocation.test.result in _result.interpret_result(invocation.test.result) call - but we do not have the same way for test checks, and as I was saying, their interpretation needs to be given to interpret_result(), as soon as you try to extract it from a result, you're on the wrong track.

I don't want to start rewriting the patch for you, but you obviously do not enjoy the coding at this moment, if it would help, we can sit on it.

[martinhoyer]

I for one don't think so. Where do you think it would be needed?

It's just that the ResultsInterpret is not exactly DRY.
Only thing that came to mind, except for using str literals instead of enums.

We can get on a call and try to get through the parts that cause you trouble, if you want to.

Thanks, much appreciated.
It seems like we strayed a bit too far from the original goal and while we have check result keys, it's used the same way as test result and I'm not sure if it is the best way to let users define that the check result should be "ignored".

[martinhoyer]

I don't want to start rewriting the patch for you, but you obviously do not enjoy the coding at this moment, if it would help, we can sit on it.

Thanks @happz, I'll get to it with a pair of fresh eyes later. Sorry for letting my depression leak to the comments here.

[happz]

Ok, as you wish. Just a couple of notes:

• something like this could work for propagating how check results should be interpreted - check_result = CheckResultInterpret(check.result.value) is definitely not the right way how to get this info.

  -        return _result.interpret_result(invocation.test.result)
  +        return _result.interpret_result(
  +            invocation.test.result,
  +            {
  +                check.how: check.result
  +                for check in invocation.test.check
  +            }
  +        )
  
       def interpret_result(
               self,
  -            interpret: ResultInterpret) -> 'Result':
  +            interpret_test: ResultInterpret,
  +            interpret_checks: dict[str, CheckResultInterpret]) -> 'Result':

• use a helper function for setting a note, the repetition of if/else consumes 4 lines for every single note, a helper function would take care of ', ...' and condition, greatly improving the readability of code.
• this looks like a mistake, check result propagated into result - is that correct? We probably don't want check result to be promoted to main result, instead failed results would turn the main result into a failed one - a skipped check does not mean main result should turn to SKIP

             elif interpret_check in [CheckResultInterpret.PASS, CheckResultInterpret.FAIL,
                                   CheckResultInterpret.INFO, CheckResultInterpret.WARN,
                                   CheckResultInterpret.ERROR, CheckResultInterpret.SKIP]:
                 self.result = ResultOutcome(check.value)

• the order should be probably reversed: first we need to sort out check results, because they might be PASS, but their result key might be set to fail, turning them to failed checks - any(check.result == ResultOutcome.FAIL for check in self.check) will miss checks that would turn into failed checks because of their interpretation.

[happz]

This branch does look superfluous to me: check.result should be changed, not self.result - at this point, we're deciding the fate of a single check result, and the test result is beyond the horizon. The effect of this check result on the test result will be evaluated later. Which means, the outcome of the test did not change in any way, therefore no need for a note -> no need to check for RESPECT + FAIL.

[martinhoyer]

make sense

[happz]

We don't know that yet: if the test has result: info, it does not matter what is the outcome of checks, they should never override the main test result, because the user clearly said "skip" is the desired and final value. We still need to resolve them, to record their final outcomes though, therefore we can't skip check results when result: skip is set.

I have a feeling you are getting too entangled in the problem. My advice would be to ignore the other part of the problem: focus on the interpretation of one single check result, and completely ignore what it might do to the test result. Go over all check results, and interpret every single one of them as needed. Ignore whatever their effect might be on the main result. Only after that, take self.result and interpret_test, collect the final check results, and resolve their interaction. If test.result is SKIP, PASS, etc., set self.result and exit. If it's RESPECT, compute the effect of check results, switch self.result to FAIL or WARN, exit. And so on. We are not in the business of saving CPU cycles, make it correct first, and then we can make it fast by introducing shortcuts. We can afford crude and readable code :)

[happz]

Maybe even one more: how about ignoring this method for now, and instead constructing a simple method that would focus on the interpretation only. Feed it with ResultOutcome and ResultInterpret for the test, a map of check name/ResultOutcome and a map of check name/CheckResultInterpret, and let it decide the final test result, final check results, and optionally a note. Add a parametrized unit test for it for various combinations. Do not feed it with Result or CheckResult or any structures, just enums and strings. Get that right, get it working, then we can discuss just how this process works, whether e.g. SKIP has the desired effect. No worrying about other structures. Once we are happy, translate it to this method, i.e. let it write decisions into structures.

[martinhoyer]

Thanks again. I'll get to it tomorrow.

Once we arr

🏴‍☠️

[martinhoyer]

I'll get to it tomorrow.

hmm, looks like a busy weekend. Monday it is.