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.

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

Change result note key from a string to a list of strings #3254

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 21 additions & 13 deletions spec/plans/results.fmf
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ description: |
# String, the original outcome of the test execution.
original-result: "pass"|"fail"|"info"|"warn"|"error"|"skip"

# String, optional comment to report with the result.
note: "Things were great."
# List of strings, optional comments to report with the result.
note:
- "Things were great."

# List of strings, paths to log files.
log:
Expand Down Expand Up @@ -88,8 +89,9 @@ description: |
# String, the original outcome of the check execution.
original-result: "pass"|"fail"|"info"|"warn"|"error"|"skip"

# String, optional comment to report with the result.
note: "Things were great."
# List of strings, optional comments to report with the result.
note:
- "Things were great."

# List of strings, paths to logs files.
log:
Expand Down Expand Up @@ -126,8 +128,9 @@ description: |
# String, the original outcome of the phase execution.
original-result: "pass"|"fail"|"info"|"warn"|"error"|"skip"

# String, optional comment to report with the result.
note: "Things were great."
# List of strings, optional comments to report with the result.
note:
- "Things were great."

# List of strings, paths to log files.
log:
Expand Down Expand Up @@ -269,7 +272,8 @@ example:
start-time: "2023-03-10T09:44:14.439120+00:00"
end-time: "2023-03-10T09:44:24.242227+00:00"
duration: 00:00:09
note: good result
note:
- good result
ids:
extra-nitrate: some-nitrate-id
guest:
Expand All @@ -284,7 +288,8 @@ example:
start-time: "2023-03-10T09:44:14.439120+00:00"
end-time: "2023-03-10T09:44:24.242227+00:00"
duration: 00:00:09
note: fail result
note:
- fail result
guest:
name: default-0

Expand All @@ -295,7 +300,8 @@ example:
log:
- pass_log
duration: 00:11:22
note: good result
note:
- good result
ids:
extra-nitrate: some-nitrate-id

Expand All @@ -305,7 +311,8 @@ example:
- fail_log
- another_log
duration: 00:22:33
note: fail result
note:
- fail result

- |
# Example of a perfectly valid, yet stingy custom results file
Expand All @@ -325,7 +332,8 @@ example:
start-time: "2023-03-10T09:44:14.439120+00:00"
end-time: "2023-03-10T09:44:24.242227+00:00"
duration: 00:00:09
note: good result
note:
- good result
ids:
extra-nitrate: some-nitrate-id
guest:
Expand All @@ -335,12 +343,12 @@ example:
event: after-test
result: pass
log: []
note:
note: []
- name: kernel-panic
event: after-test
result: pass
log: []
note:
note: []

- |
# syntax: json
Expand Down
5 changes: 4 additions & 1 deletion tests/execute/restraint/tmt-abort/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,11 @@ rlJournalStart
rlAssertNotGrep "This test should not be executed." $rlRun_LOG
rlAssertNotGrep "This should not be executed either." $rlRun_LOG


rlAssertGrep "result: error" "${run}/plan/execute/results.yaml"
rlAssertGrep "note: aborted" "${run}/plan/execute/results.yaml"
rlAssertEquals "results should record the test aborted" \
"$(yq -r '.[] | .note | join(", ")' ${run}/plan/execute/results.yaml)" \
"beakerlib: State 'started', aborted"
rlPhaseEnd

rlPhaseStartCleanup
Expand Down
8 changes: 6 additions & 2 deletions tests/execute/result/custom/results.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
{
"name": "/test/passing",
"result": "pass",
"note": "good result",
"note": [
"good result"
],
"log": [
"pass_log"
],
Expand All @@ -28,7 +30,9 @@
"another-id": "bar2"
},
"duration": "00:23:34",
"note": "fail result",
"note": [
"fail result"
],
"serial-number": 2,
"guest": {
"name": "client-1",
Expand Down
6 changes: 4 additions & 2 deletions tests/execute/result/custom/results.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
- name: /test/passing
result: pass
note: good result
note:
- good result
log:
- pass_log
ids:
Expand All @@ -20,7 +21,8 @@
some-id: foo2
another-id: bar2
duration: 00:22:33
note: fail result
note:
- fail result
serial-number: 2
guest:
name: client-1
Expand Down
14 changes: 7 additions & 7 deletions tmt/frameworks/beakerlib.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import re
from typing import TYPE_CHECKING, Optional
from typing import TYPE_CHECKING

import tmt.base
import tmt.log
Expand Down Expand Up @@ -56,7 +56,7 @@ def extract_results(
logger: tmt.log.Logger) -> list[tmt.result.Result]:
""" Check result of a beakerlib test """
# Initialize data, prepare log paths
note: Optional[str] = None
note: list[str] = []
log: list[Path] = []
for filename in [tmt.steps.execute.TEST_OUTPUT_FILENAME, 'journal.txt']:
if (invocation.path / filename).is_file():
Expand All @@ -69,7 +69,7 @@ def extract_results(
results = invocation.phase.read(beakerlib_results_filepath, level=3)
except tmt.utils.FileError:
logger.debug(f"Unable to read '{beakerlib_results_filepath}'.", level=3)
note = 'beakerlib: TestResults FileError'
note.append('beakerlib: TestResults FileError')

return [tmt.Result.from_test_invocation(
invocation=invocation,
Expand All @@ -93,7 +93,7 @@ def extract_results(
logger.debug(
f"No '{missing_piece}' found in '{beakerlib_results_filepath}'{hint}.",
level=3)
note = 'beakerlib: Result/State missing'
note.append('beakerlib: Result/State missing')
return [tmt.Result.from_test_invocation(
invocation=invocation,
result=ResultOutcome.ERROR,
Expand All @@ -106,15 +106,15 @@ def extract_results(
# Check if it was killed by timeout (set by tmt executor)
actual_result = ResultOutcome.ERROR
if invocation.return_code == tmt.utils.ProcessExitCodes.TIMEOUT:
note = 'timeout'
note.append('timeout')
invocation.phase.timeout_hint(invocation)

elif tmt.utils.ProcessExitCodes.is_pidfile(invocation.return_code):
note = 'pidfile locking'
note.append('pidfile locking')

# Test results should be in complete state
elif state != 'complete':
note = f"beakerlib: State '{state}'"
note.append(f"beakerlib: State '{state}'")
# Finally we have a valid result
else:
actual_result = ResultOutcome.from_spec(result.lower())
Expand Down
6 changes: 3 additions & 3 deletions tmt/frameworks/shell.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def extract_results(
logger: tmt.log.Logger) -> list[tmt.result.Result]:
""" Check result of a shell test """
assert invocation.return_code is not None
note = None
note: list[str] = []

try:
# Process the exit code and prepare the log path
Expand All @@ -39,11 +39,11 @@ def extract_results(
result = ResultOutcome.ERROR
# Add note about the exceeded duration
if invocation.return_code == tmt.utils.ProcessExitCodes.TIMEOUT:
note = 'timeout'
note.append('timeout')
invocation.phase.timeout_hint(invocation)

elif tmt.utils.ProcessExitCodes.is_pidfile(invocation.return_code):
note = 'pidfile locking'
note.append('pidfile locking')

return [tmt.Result.from_test_invocation(
invocation=invocation,
Expand Down
26 changes: 12 additions & 14 deletions tmt/result.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,9 @@ class BaseResult(SerializableContainer):
serialize=lambda result: result.value,
unserialize=ResultOutcome.from_spec
)
note: Optional[str] = None
note: list[str] = field(
default_factory=cast(Callable[[], list[str]], list),
unserialize=lambda value: [] if value is None else value)
log: list[Path] = field(
default_factory=cast(Callable[[], list[Path]], list),
serialize=lambda logs: [str(log) for log in logs],
Expand All @@ -175,10 +177,14 @@ def show(self) -> str:
]

if self.note:
components.append(f'({self.note})')
components.append(f'({self.printable_note})')

return ' '.join(components)

@property
def printable_note(self) -> str:
return ', '.join(self.note)


@dataclasses.dataclass
class CheckResult(BaseResult):
Expand Down Expand Up @@ -267,7 +273,7 @@ def from_test_invocation(
*,
invocation: 'tmt.steps.execute.TestInvocation',
result: ResultOutcome,
note: Optional[str] = None,
note: Optional[list[str]] = None,
ids: Optional[ResultIds] = None,
log: Optional[list[Path]] = None) -> 'Result':
"""
Expand Down Expand Up @@ -310,7 +316,7 @@ def from_test_invocation(
fmf_id=invocation.test.fmf_id,
context=invocation.phase.step.plan._fmf_context,
result=result,
note=note,
note=note or [],
start_time=invocation.start_time,
end_time=invocation.end_time,
duration=invocation.real_duration,
Expand All @@ -336,15 +342,7 @@ def interpret_result(self, interpret: ResultInterpret) -> 'Result':
return self

# Extend existing note or set a new one
if self.note:
self.note += f', original result: {self.result.value}'

elif self.note is None:
self.note = f'original result: {self.result.value}'

else:
raise tmt.utils.SpecificationError(
f"Test result note '{self.note}' must be a string.")
self.note.append(f'original result: {self.result.value}')

if interpret == ResultInterpret.XFAIL:
# Swap just fail<-->pass, keep the rest as is (info, warn,
Expand Down Expand Up @@ -416,7 +414,7 @@ def show(self, display_guest: bool = True) -> str:
components.append(f'(on {format_guest_full_name(self.guest.name, self.guest.role)})')

if self.note:
components.append(f'({self.note})')
components.append(f'({self.printable_note})')

return ' '.join(components)

Expand Down
4 changes: 3 additions & 1 deletion tmt/schemas/common.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,9 @@ definitions:
- restraint

result_note:
type: string
type: array
items:
type: string

result_log:
type: array
Expand Down
13 changes: 5 additions & 8 deletions tmt/steps/execute/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ def _process_results_reduce(
# The original one may be left unset - malformed results file,
# for example, provides no usable original outcome.
actual_outcome: ResultOutcome
note: Optional[str] = None
note: list[str] = []

try:
outcomes = [
Expand All @@ -816,7 +816,7 @@ def _process_results_reduce(

except tmt.utils.SpecificationError as exc:
actual_outcome = ResultOutcome.ERROR
note = exc.message
note.append(exc.message)

else:
hierarchy = [
Expand Down Expand Up @@ -883,10 +883,7 @@ def _process_results_partials(

else:
if not partial_result.name.startswith('/'):
if partial_result.note and isinstance(partial_result.note, str):
partial_result.note += ", custom test result name should start with '/'"
else:
partial_result.note = "custom test result name should start with '/'"
partial_result.note.append("custom test result name should start with '/'")
partial_result.name = '/' + partial_result.name
partial_result.name = test.name + partial_result.name

Expand Down Expand Up @@ -937,13 +934,13 @@ def extract_custom_results(self, invocation: TestInvocation) -> list["tmt.Result
if not collection.file_exists:
return [tmt.Result.from_test_invocation(
invocation=invocation,
note=f"custom results file not found in '{invocation.test_data_path}'",
note=[f"custom results file not found in '{invocation.test_data_path}'"],
result=ResultOutcome.ERROR)]

if not collection.results:
return [tmt.Result.from_test_invocation(
invocation=invocation,
note="no custom results were provided",
note=["no custom results were provided"],
result=ResultOutcome.ERROR)]

collection.validate()
Expand Down
8 changes: 4 additions & 4 deletions tmt/steps/execute/internal.py
Original file line number Diff line number Diff line change
Expand Up @@ -555,12 +555,12 @@ def _run_tests(
except tmt.utils.RebootTimeoutError:
for result in invocation.results:
result.result = ResultOutcome.ERROR
result.note = 'reboot timeout'
result.note.append('reboot timeout')

else:
for result in invocation.results:
result.result = ResultOutcome.ERROR
result.note = 'crashed too many times'
result.note.append('crashed too many times')

# Handle reboot, abort, exit-first
if invocation.reboot_requested:
Expand All @@ -573,12 +573,12 @@ def _run_tests(
except tmt.utils.RebootTimeoutError:
for result in invocation.results:
result.result = ResultOutcome.ERROR
result.note = 'reboot timeout'
result.note.append('reboot timeout')

if invocation.abort_requested:
for result in invocation.results:
# In case of aborted all results in list will be aborted
result.note = 'aborted'
result.note.append('aborted')

self._results.extend(invocation.results)

Expand Down
2 changes: 1 addition & 1 deletion tmt/steps/report/html/template.html.j2
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ Context:
{% endfor %}
</td>
<td class="data"><a href="{{ base_dir | linkable_path | urlencode }}/{{ result.data_path | urlencode }}">data</a></td>
<td class="note">{% if result.note %}{{ result.note | e }}{% else %}-{% endif %}</td>
<td class="note">{% if result.note %}{{ result.printable_note | e }}{% else %}-{% endif %}</td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is printable_note property actually needed? I think we should maybe print the notes as HTML list and keep the display/design of these notes on the Jinja template, e.g.:

{% if result.note %}
<ul>
{% for note in result.note %}
<li>{{ note | e }}</li>
{% endfor %}
</ul>
{% else %}
-
{% endif %}

Copy link
Collaborator

@martinhoyer martinhoyer Nov 14, 2024

Choose a reason for hiding this comment

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

No strong opinion, but would prefer printable_note approach myself.
(huh, it looks weird when you write comment using the "Start Review" button..)

<td class="action">
{% if result.check %}
<button onclick="toggle_row_visibility(this, 'check-{{ loop.index }}')" title="Show / hide checks">checks&nbsp;[+]</button>
Expand Down
Loading