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.

[martinhoyer]

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

perplexing

[martinhoyer]

Trying to implement the check results as discussed yesterday. Started from scratch. Still don't understand specs, schemas and docs generation. Why can't we just have a one place to define things at?

We can. It somehow started separately, in an organic way, as projects grow, and the amount of work necessary to turn the tide and introduce a single source of truth is not trivial. The work is ongoing, and one possible answer might be that I, who also dislike the currently fractured state of things, am unable to change it all in one night.

Right, but isn't that a sunk cost fallacy? I'm not saying you should do it alone in one night, but perhaps have a defined "state" to strive for and think about whether it wouldn't be worth working towards it in a long run.

[happz]

Trying to implement the check results as discussed yesterday. Started from scratch. Still don't understand specs, schemas and docs generation. Why can't we just have a one place to define things at?

We can. It somehow started separately, in an organic way, as projects grow, and the amount of work necessary to turn the tide and introduce a single source of truth is not trivial. The work is ongoing, and one possible answer might be that I, who also dislike the currently fractured state of things, am unable to change it all in one night.

Right, but isn't that a sunk cost fallacy? I'm not saying you should do it alone in one night, but perhaps have a defined "state" to strive for and think about whether it wouldn't be worth working towards it in a long run.

Yeah, that was just an example. There is a long-term goal, it just takes so many small steps to get there. Just recently plugin docs began to be generated from plugin sources. It's useful on its own, but it also served as an experiment on what would we need to render plugin schema from plugin sources. And so on, we're moving toward a much simpler state of things, with fewer sources of truth, it's just very slow.

[martinhoyer]

Not at all sure about what's happening in execute/internal

[martinhoyer]

Does it make sense to move it to test.yaml, or is check expected to be used elsewhere in future?

[sbertramrh]

I am hoping we can move the triggering of check to the plan, with the expectation that the check will operate on each test in the plan. But I am not sure if that work has been green lighted yet.

[martinhoyer]

Ugly workaround.
@happz Do you have any cleaner solution to point me to please? I spent way more time than I'd like to admit trying to solve this.

[happz]

Yeah, this is not very nice :/ What fields forced you to take this road? result is the new one, but it's a string, like how, so I don't immediately recall what might be a problem. What's logged by the script?

[happz]

Nevermind, I can try it out.

[martinhoyer]

It says CheckResultInterpret.RESPECT is the 'actual_default'. I've tried to add exporter=lambda result: result.value to Check.result field, but to no avail.

[happz]

Yep, got it, it's an enum, I suppose it's the first one in plugins, and we can solve it once and for all :)

diff --git a/docs/scripts/generate-plugins.py b/docs/scripts/generate-plugins.py
index a37d83c1..9368f731 100755
--- a/docs/scripts/generate-plugins.py
+++ b/docs/scripts/generate-plugins.py
@@ -1,6 +1,7 @@
 #!/usr/bin/env python3
 
 import dataclasses
+import enum
 import sys
 import textwrap
 from typing import Any
@@ -106,6 +107,12 @@ def container_intrinsic_fields(container: ContainerClass) -> list[str]:
     return field_names
 
 
+def is_enum(value: Any) -> bool:
+    """ Find out whether a given value is an enum member """
+
+    return isinstance(value, enum.Enum)
+
+
 def _create_step_plugin_iterator(registry: tmt.plugins.PluginRegistry[tmt.steps.Method]):
     """ Create iterator over plugins of a given registry """
 
@@ -184,6 +191,7 @@ def main() -> None:
         STEP=step_name,
         PLUGINS=plugin_generator,
         REVIEWED_PLUGINS=REVIEWED_PLUGINS,
+        is_enum=is_enum,
         container_fields=tmt.utils.container_fields,
         container_field=tmt.utils.container_field,
         container_ignored_fields=container_ignored_fields,
diff --git a/docs/templates/plugins.rst.j2 b/docs/templates/plugins.rst.j2
index 5b99149c..5969cc4c 100644
--- a/docs/templates/plugins.rst.j2
+++ b/docs/templates/plugins.rst.j2
@@ -30,7 +30,7 @@
             {% else %}
     Default: {% for default_item in actual_default %}``{{ default_item.pattern | default(default_item) }}``{% if not loop.last %}, {% endif %}{% endfor %}
             {% endif %}
-        {% elif actual_default.__class__.__name__ == 'CheckResultInterpret' %}
+        {% elif is_enum(actual_default) %}
     Default: ``{{ actual_default.value }}``
         {% else %}
             {% set _ = LOGGER.warn("%s/%s.%s: could not render default value, '%s'" | format(STEP, plugin_id, field_name, actual_default), shift=0) %}

[martinhoyer]

Nice, thanks.

[martinhoyer]

@psss @happz I've marked the 'outdated' discussions as resolved, as it got quite messy with all the refactoring - sorry. I believe all the points have been incorporated though.

[martinhoyer]

/packit build

[martinhoyer]

Are we ok with this? I see no reason to add note when original and actual results are the same, no?

[martinhoyer]

This way doesn't really work when multiple check.how/check.name are the same. There surely has to be a better way to add/match CheckResultInterpret to each Check?

[happz]

I believe we spoke about a way: there is a trivial mapping between a result and its parent test, TestInvocation stores results & points to the test. We did not needed to transition from check result to a check, therefore there is no such mapping yet, we need to establish one. My proposal was to add a dedicated mapping to TestInvocation class which would map from CheckResult to Check instances, and it would be populated in _run_checks_for_test() (most likely), as that is the place where check results are born. Something like this:

diff --git a/tmt/steps/execute/__init__.py b/tmt/steps/execute/__init__.py
index 407b4ac0..7f77d179 100644
--- a/tmt/steps/execute/__init__.py
+++ b/tmt/steps/execute/__init__.py
@@ -16,6 +16,7 @@ import fmf.utils
 
 import tmt
 import tmt.base
+import tmt.checks
 import tmt.log
 import tmt.steps
 import tmt.utils
@@ -175,6 +176,9 @@ class TestInvocation:
     results: list[Result] = dataclasses.field(default_factory=list)
     check_results: list[CheckResult] = dataclasses.field(default_factory=list)
 
+    check_result_to_check: dict[CheckResult, tmt.checks.Check] = dataclasses.field(
+        default_factory=dict)
+
     check_data: dict[str, Any] = field(default_factory=dict)
 
     return_code: Optional[int] = None
@@ -954,6 +958,8 @@ class ExecutePlugin(tmt.steps.Plugin[ExecuteStepDataT, None]):
                 result.end_time = format_timestamp(timer.end_time)
                 result.duration = format_duration(timer.duration)
 
+                invocation.check_result_to_check[result] = check
+
             results += check_results
 
         return results

It's not clean, it's not nice, I'd like to make it look and feel like the test invocation, but I don't have that ready and I promise to polish it later. With this, the following should work: use this mapping to collect check result/interpretation pairs, and use the right one for the given check result when calling the interpretation method:

interpret_checks = {
    check_result: check.result for check_result, check in invocation.check_result_to_check.items()
}

...

check_result.interpret_check_result(interpret_checks[check_result]))

[martinhoyer]

I was thinking about adding a logic that would check for both before and after and make the xfail 'pass' only if after or both are failed, but wasn't sure if it's needed. ?

[martinhoyer]

On second thought, this does need to be improved.
So, what are the expectations?
before fails > pass
before pass > pass
after fails > pass
after fails > fail
?