Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions tests/execute/result/check_results.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
#!/bin/bash
. /usr/share/beakerlib/beakerlib.sh || exit 1

rlJournalStart
rlPhaseStartSetup
rlRun "tmp=\$(mktemp -d)" 0 "Creating tmp directory"
rlRun "pushd $tmp"
rlRun "set -o pipefail"
rlPhaseEnd

rlPhaseStartTest "Test with passing checks"
rlRun -s "tmt run -vvv test --name /test/check-pass"
rlAssertGrep "pass /test/check-pass" $rlRun_LOG
rlAssertGrep "pass dmesg" $rlRun_LOG
rlPhaseEnd

rlPhaseStartTest "Test with failing dmesg check (respect)"
rlRun -s "tmt run -vvv test --name /test/check-fail-respect" 2
rlAssertGrep "fail /test/check-fail-respect" $rlRun_LOG
rlAssertGrep "fail dmesg" $rlRun_LOG
rlPhaseEnd

rlPhaseStartTest "Test with failing dmesg check (info)"
rlRun -s "tmt run -vvv test --name /test/check-fail-info"
rlAssertGrep "pass /test/check-fail-info" $rlRun_LOG
rlAssertGrep "skip dmesg" $rlRun_LOG
rlPhaseEnd

rlPhaseStartTest "Test with passing dmesg check (xfail)"
rlRun -s "tmt run -vvv test --name /test/check-xfail-pass" 2
rlAssertGrep "fail /test/check-xfail-pass" $rlRun_LOG
rlAssertGrep "pass dmesg" $rlRun_LOG
rlPhaseEnd

rlPhaseStartTest "Test with failing dmesg check (xfail)"
rlRun -s "tmt run -vvv test --name /test/check-xfail-fail"
rlAssertGrep "pass /test/check-xfail-fail" $rlRun_LOG
rlAssertGrep "fail dmesg" $rlRun_LOG
rlPhaseEnd

rlPhaseStartTest "Test with multiple checks with different result interpretations"
rlRun -s "tmt run -vvv test --name /test/check-multiple" 2
rlAssertGrep "fail /test/check-multiple" $rlRun_LOG
rlAssertGrep "fail dmesg" $rlRun_LOG
rlAssertGrep "pass dmesg" $rlRun_LOG
rlAssertGrep "info dmesg" $rlRun_LOG
rlPhaseEnd

rlPhaseStartTest "Test with failing dmesg check but overridden by test result"
rlRun -s "tmt run -vvv test --name /test/check-override"
rlAssertGrep "pass /test/check-override" $rlRun_LOG
rlAssertGrep "fail dmesg" $rlRun_LOG
rlPhaseEnd

rlPhaseStartCleanup
rlRun "popd"
rlRun "rm -r $tmp" 0 "Removing tmp directory"
rlPhaseEnd
rlJournalEnd
83 changes: 83 additions & 0 deletions tests/execute/result/check_results/test.fmf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
summary: Tests for check results behaviour
description: Verify that check results, including after-test checks, are correctly handled

/test/check-pass:
summary: Test with passing checks
test: echo "Test passed"
framework: shell
duration: 1m
check:
- how: dmesg
result: respect
# Expected outcome: PASS (test passes, check passes)

/test/check-fail-respect:
summary: Test with failing dmesg check (respect)
test: echo "Test passed"
framework: shell
duration: 1m
check:
- how: dmesg
failure-pattern: '.*'
result: respect
# Expected outcome: FAIL (test passes, but check fails and is respected)

/test/check-fail-info:
summary: Test with failing dmesg check (info)
test: echo "Test passed"
framework: shell
duration: 1m
check:
- how: dmesg
failure-pattern: '.*'
result: skip
# Expected outcome: PASS (test passes, check fails, but should be just info)

/test/check-xfail-pass:
summary: Test with passing dmesg check (xfail)
test: echo "Test passed"
framework: shell
duration: 1m
check:
- how: dmesg
result: xfail
# Expected outcome: FAIL (test passes, check passes but xfail expects it to fail)

/test/check-xfail-fail:
summary: Test with failing dmesg check (xfail)
test: echo "Test passed"
framework: shell
duration: 1m
check:
- how: dmesg
failure-pattern: '.*'
result: xfail
# Expected outcome: PASS (test passes, check fails but xfail expects it to fail)

/test/check-multiple:
summary: Test with multiple checks with different result interpretations
test: echo "Test passed"
framework: shell
duration: 1m
check:
- how: dmesg
failure-pattern: '.*'
result: respect
- how: dmesg
result: xfail
- how: dmesg
failure-pattern: '.*'
result: info
# Expected outcome: FAIL (first dmesg check fails and is respected, second dmesg check passes but xfail expects it to fail, third failing dmesg check is just info)

/test/check-override:
summary: Test with failing dmesg check but overridden by test result
test: echo "Test passed"
framework: shell
duration: 1m
result: pass
check:
- how: dmesg
failure-pattern: '.*'
result: respect
# Expected outcome: PASS (test passes, check fails but is overridden by 'result: pass')
3 changes: 3 additions & 0 deletions tests/execute/result/main.fmf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@
/special:
summary: Test special characters generated to tmt-report-results.yaml
test: ./special.sh
/check_results:
summary: Test behaviour of check results, including after-test checks
test: ./check_results.sh
25 changes: 9 additions & 16 deletions tmt/checks/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import dataclasses
import enum
import functools
from typing import TYPE_CHECKING, Any, Callable, Generic, Optional, TypedDict, TypeVar, cast

import tmt.log
import tmt.steps.provision
import tmt.utils
from tmt.checks.enums import CheckEvent, CheckResultInterpret
from tmt.plugins import PluginRegistry
from tmt.utils import (
NormalizeKeysMixin,
Expand Down Expand Up @@ -67,20 +67,7 @@ def find_plugin(name: str) -> 'CheckPluginClass':
class _RawCheck(TypedDict):
how: str
enabled: bool


class CheckEvent(enum.Enum):
""" Events in test runtime when a check can be executed """

BEFORE_TEST = 'before-test'
AFTER_TEST = 'after-test'

@classmethod
def from_spec(cls, spec: str) -> 'CheckEvent':
try:
return CheckEvent(spec)
except ValueError:
raise tmt.utils.SpecificationError(f"Invalid test check event '{spec}'.")
result: str


@dataclasses.dataclass
Expand All @@ -100,6 +87,12 @@ class Check(
default=True,
is_flag=True,
help='Whether the check is enabled or not.')
result: CheckResultInterpret = field(
default=CheckResultInterpret.RESPECT,
help='How to interpret the check result.',
serialize=lambda result: result.value,
unserialize=CheckResultInterpret.from_spec
)

@functools.cached_property
def plugin(self) -> 'CheckPluginClass':
Expand Down Expand Up @@ -228,7 +221,7 @@ def normalize_test_check(
if isinstance(raw_test_check, str):
try:
return CheckPlugin.delegate(
raw_data={'how': raw_test_check, 'enabled': True},
raw_data={'how': raw_test_check, 'enabled': True, 'result': 'respect'},
logger=logger)

except Exception as exc:
Expand Down
28 changes: 28 additions & 0 deletions tmt/checks/enums.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import enum
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why keep them in a standalone file? __init__.py has about 250+ lines, isn't that small enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just needed to deal with that circular import somehow. Would gladly do a less bad solution.



class CheckEvent(enum.Enum):
""" Events in test runtime when a check can be executed """

BEFORE_TEST = 'before-test'
AFTER_TEST = 'after-test'

@classmethod
def from_spec(cls, spec: str) -> 'CheckEvent':
try:
return CheckEvent(spec)
except ValueError:
raise ValueError(f"Invalid test check event '{spec}'.")


class CheckResultInterpret(enum.Enum):
INFO = 'info'
RESPECT = 'respect'
XFAIL = 'xfail'

@classmethod
def from_spec(cls, spec: str) -> 'CheckResultInterpret':
try:
return CheckResultInterpret(spec)
except ValueError:
raise ValueError(f"Invalid check result interpretation '{spec}'.")
38 changes: 37 additions & 1 deletion tmt/result.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import tmt.identifier
import tmt.log
import tmt.utils
from tmt.checks import CheckEvent
from tmt.checks.enums import CheckEvent, CheckResultInterpret
from tmt.utils import GeneralError, Path, SerializableContainer, field

if TYPE_CHECKING:
Expand Down Expand Up @@ -189,6 +189,27 @@ class CheckResult(BaseResult):
default=CheckEvent.BEFORE_TEST,
serialize=lambda event: event.value,
unserialize=CheckEvent.from_spec)
check_result: CheckResultInterpret = field(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result class cannot contain a field that governs its own interpretation. That is owned by a different entity: Test.result defines how Result should be interpreted, similarly Check.result defines how CheckResult should be interpreted. This field should not exist, check result simply cannot decide itself, it must be told how to interpret the value.

    def interpret_check_result(self, interpret: CheckResultInterpret) -> 'CheckResult':
        """Interpret check result according to the check_result interpretation."""

        if interpret == CheckResultInterpret.RESPECT:
            return self

        if interpret == CheckResultInterpret.INFO:
            self.result = ResultOutcome.INFO
        elif interpret == CheckResultInterpret.XFAIL:
            self.result = {
                ResultOutcome.FAIL: ResultOutcome.PASS,
                ResultOutcome.PASS: ResultOutcome.FAIL
                }.get(self.result, self.result)

        return self

Take a CheckResult instance, find the corresponding Check, most likely from the invocation.test.check field, and pass its result to CheckResult.interpret_check_result(). Recall the mapping I proposed last week to be put into Result.interpret_result():

{
    check.how: check.result
    for check in invocation.test.check
}

It's the same solution, the idea is there: the interpretation does not lie in the result itself, it's owned by test (for test results) or test check (for test check results), and must be extracted from them (which is exactly what Result.from_test_invocation() does, return _result.interpret_result(invocation.test.result)).

default=CheckResultInterpret.RESPECT,
serialize=lambda result: result.value,
unserialize=CheckResultInterpret.from_spec
)

def interpret_check_result(self) -> 'CheckResult':
"""Interpret check result according to the check_result interpretation."""

if self.check_result == CheckResultInterpret.RESPECT:
return self

if self.check_result == CheckResultInterpret.INFO:
self.result = ResultOutcome.INFO
elif self.check_result == CheckResultInterpret.XFAIL:
self.result = {
ResultOutcome.FAIL: ResultOutcome.PASS,
ResultOutcome.PASS: ResultOutcome.FAIL
}.get(self.result, self.result)

return self


@dataclasses.dataclass
Expand Down Expand Up @@ -347,6 +368,21 @@ def interpret_result(self, interpret: ResultInterpret) -> 'Result':
raise tmt.utils.SpecificationError(
f"Test result note '{self.note}' must be a string.")

# Interpret check results
failed_checks: list[CheckResult] = []
for check in self.check:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, for check_result in self.check - check might be very misleading, especially when you do need to access actual checks and their result keys.

interpreted_check = check.interpret_check_result()
if (
interpreted_check.result == ResultOutcome.FAIL and
interpreted_check.check_result != CheckResultInterpret.INFO
):
failed_checks.append(interpreted_check)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need a bit more work, because self.check is left untouched: interpretation happens, but interpreted_check is then thrown away, it's not stored anywhere. It's basically swapping one list for another, simple list comprehension. Together with the necessity of accessing checks to get their result key, it might be something like the following:

# This is constructed in `from_test_invocation()` and fed to `interpret_result()` - just like the result interpretation:
{
    check.how: check.result
    for check in invocation.test.check
}


def interpret_result(self, interpret: ResultInterpret, interpret_checks: dict[str, CheckResultInterpret]) -> 'Result':
...

self.check = [
    check_result.interpret_check_result(interpret_checks[check_result.name])
    for check_result in self.check
]

failed_checks = [
    check_result
    for check_result in self.check
    if check_result.outcome == ResultOutcome.FAIL]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. Needs work, yes. Though I might need someone to go through the code with me, because most of the time I'm just guessing what is supposed to do what and how and then trying a blind trial-error. Not enjoyable and I'm back to feeling like an useless idiot.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I did offer to have an online session, and the offer still stands. Add something to my calendar and we can try it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, much appreciated.


if failed_checks:
self.result = ResultOutcome.FAIL
check_note = ', '.join([f"Check '{check.name}' failed" for check in failed_checks])
self.note = f"{self.note}, {check_note}" if self.note else check_note

if interpret == ResultInterpret.XFAIL:
# Swap just fail<-->pass, keep the rest as is (info, warn,
# error)
Expand Down
11 changes: 6 additions & 5 deletions tmt/steps/execute/internal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,17 +453,18 @@ def _save_process(
# losing logs if the guest becomes later unresponsive.
guest.pull(source=self.step.plan.data_directory)

# Extract test results and store them in the invocation. Note
# that these results will be overwritten with a fresh set of
# results after a successful reboot in the middle of a test.
invocation.results = self.extract_results(invocation, logger)

# Run after-test checks before extracting results
invocation.check_results += self.run_checks_after_test(
invocation=invocation,
environment=environment,
logger=logger
)

# Extract test results and store them in the invocation. Note
# that these results will be overwritten with a fresh set of
# results after a successful reboot in the middle of a test.
invocation.results = self.extract_results(invocation, logger)

if invocation.is_guest_healthy:
# Fetch #2: after-test checks might have produced remote files as well,
# we need to fetch them too.
Expand Down
Loading