checks: make suite failure reporting configurable#2418
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable max_reported_failures parameter to the Suite and SuiteResult classes, allowing users to control the number of failed scenarios displayed in reports. The default is set to 20, and the logic in rich_console has been updated to respect this setting. Feedback includes suggestions to allow a minimum value of 0 for complete suppression of failure details, refactoring duplicated logic in the reporting output, and merging redundant imports in the test suite.
| "Maximum number of failed or errored scenarios to show in the rich " | ||
| "report. Use None to show all." | ||
| ), | ||
| ge=1, |
There was a problem hiding this comment.
Consider changing the minimum value to 0. Setting max_reported_failures=0 is a valid way to suppress detailed failure logs while still showing the summary count (e.g., '... and 20 more').
| ge=1, | |
| ge=0, |
| if failures_and_errors: | ||
| n_loggable_failures = 20 # TODO: make this configurable | ||
| max_reported_failures = self.max_reported_failures | ||
| reported_failures = ( | ||
| failures_and_errors | ||
| if max_reported_failures is None | ||
| else failures_and_errors[:max_reported_failures] | ||
| ) | ||
|
|
||
| # Details | ||
| yield Rule("FAILURES", characters="=", style="grey") | ||
| for f in failures_and_errors[:n_loggable_failures]: | ||
| for f in reported_failures: | ||
| yield Panel( | ||
| f, | ||
| title=f.scenario_name, | ||
| border_style=f"{STATUS_MAPPING[f.status]['color']} bold", | ||
| ) | ||
| if len(failures_and_errors) > n_loggable_failures: | ||
| yield f" ... and {len(failures_and_errors) - n_loggable_failures} more" | ||
| if len(failures_and_errors) > len(reported_failures): | ||
| yield f" ... and {len(failures_and_errors) - len(reported_failures)} more" | ||
|
|
||
| # Summary | ||
| yield Rule("SUMMARY", characters="=", style="grey") | ||
| for f in failures_and_errors[:n_loggable_failures]: | ||
| for f in reported_failures: | ||
| status = STATUS_MAPPING[f.status] | ||
| yield f"[{status['color']} bold]{f.scenario_name}[/{status['color']} bold]\t[{status['color']}]{f.status.value.upper()}[/{status['color']}]" | ||
| for tc in f.failures_and_errors: | ||
| for c in tc.failures_and_errors: | ||
| yield from ( | ||
| f"\t{line}" for line in c.__rich_console__(console, options) | ||
| ) | ||
| if len(failures_and_errors) > n_loggable_failures: | ||
| yield f" ... and {len(failures_and_errors) - n_loggable_failures} more" | ||
| if len(failures_and_errors) > len(reported_failures): | ||
| yield f" ... and {len(failures_and_errors) - len(reported_failures)} more" |
There was a problem hiding this comment.
The logic for calculating the number of hidden failures and yielding the 'more' message is duplicated. Extracting this into a variable improves maintainability and readability.
if failures_and_errors:
max_reported_failures = self.max_reported_failures
reported_failures = (
failures_and_errors
if max_reported_failures is None
else failures_and_errors[:max_reported_failures]
)
n_hidden = len(failures_and_errors) - len(reported_failures)
# Details
yield Rule("FAILURES", characters="=", style="grey")
for f in reported_failures:
yield Panel(
f,
title=f.scenario_name,
border_style=f"{STATUS_MAPPING[f.status]['color']} bold",
)
if n_hidden > 0:
yield f" ... and {n_hidden} more"
# Summary
yield Rule("SUMMARY", characters="=", style="grey")
for f in reported_failures:
status = STATUS_MAPPING[f.status]
yield f"[{status['color']} bold]{f.scenario_name}[/{status['color']} bold]\t[{status['color']}]{f.status.value.upper()}[/{status['color']}]"
for tc in f.failures_and_errors:
for c in tc.failures_and_errors:
yield from (
f"\t{line}" for line in c.__rich_console__(console, options)
)
if n_hidden > 0:
yield f" ... and {n_hidden} more"| "Maximum number of failed or errored scenarios to show in suite " | ||
| "reports. Use None to show all." | ||
| ), | ||
| ge=1, |
There was a problem hiding this comment.
Consider changing the minimum value to 0 to allow users to suppress failure details entirely if desired, consistent with the suggested change in SuiteResult.
| ge=1, | |
| ge=0, |
| from giskard.checks.core.interaction import Trace | ||
| from giskard.checks.core.result import ( | ||
| CheckResult, | ||
| ScenarioResult, | ||
| SuiteResult, | ||
| ) | ||
| from giskard.checks.core.result import ( | ||
| TestCaseResult as CheckTestCaseResult, | ||
| ) |
There was a problem hiding this comment.
These imports from the same module can be merged for better readability.
from giskard.checks.core.interaction import Trace
from giskard.checks.core.result import (
CheckResult,
ScenarioResult,
SuiteResult,
TestCaseResult as CheckTestCaseResult,
)
davidberenstein1957
left a comment
davidberenstein1957
left a comment
There was a problem hiding this comment.
instead of this argument, could you define this as an overridable environment variable and simplify the code implementation and tests?
2 participants
#2417
Summary
This PR makes
SuiteResultfailure reporting configurable instead of hard-coding the rich-report output to 20 failed/erroredscenarios.
Changes
max_reported_failurestoSuitewith a default of20max_reported_failurestoSuiteResultwith support forNoneto show all failuresSuite.run()to propagate the configured value into the returnedSuiteResultSuiteResult.__rich_console__to honor the configured limit in both the detailed and summary sectionsSuite.run()None