OPRUN-4587: Make OTE local output easier to read#704
Open
camilamacedo86 wants to merge 1 commit intoopenshift:mainfrom
Open
OPRUN-4587: Make OTE local output easier to read#704camilamacedo86 wants to merge 1 commit intoopenshift:mainfrom
camilamacedo86 wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a developer-focused local test mode: new Makefile targets, dev-only Cobra commands compiled under the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as Cobra CLI
participant Registry as Component Registry
participant Runner as Suite/Specs Runner
participant Writer as CleanResult Writer
participant Formatter as Output Formatter
participant Stdout as stdout
User->>CLI: invoke run-suite-dev SUITE
CLI->>CLI: setup context + signal handlers
CLI->>Registry: resolve component by name
Registry-->>CLI: component
CLI->>Runner: fetch suite specs (filter by qualifiers)
Runner-->>CLI: specs
CLI->>Writer: NewCleanResultWriter(stdout)
Writer->>Formatter: NewFormatter(stdout) / PrintHeader
Writer->>Runner: SetTotalTests(total)
CLI->>Runner: Run specs with concurrency (ctx)
loop per test
Runner->>Writer: Write(ExtensionTestResult)
Writer->>Formatter: PrintTestStart / PrintTestResult
Formatter->>Stdout: colorized progress lines
end
Runner-->>CLI: execution complete / error
CLI->>Writer: Flush()
Writer->>Formatter: PrintSummary
Formatter->>Stdout: final summary / failures
CLI-->>User: exit (0 success, 1 failures, 130 on forced interrupt)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: camilamacedo86 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
openshift/tests-extension/hack/run-tests-clean.sh (1)
46-46: Quote temp-file references and use deferred trap expansion.Line 46 and the unquoted
$TMPFILEusages can break on unexpected path characters and are easy to harden with quoting.Proposed patch
-TMPFILE=$(mktemp) -trap "rm -f $TMPFILE" EXIT +TMPFILE=$(mktemp) +trap 'rm -f "$TMPFILE"' EXIT @@ -./bin/olmv1-tests-ext "$@" 2>&1 | tee $TMPFILE | while IFS= read -r line; do +./bin/olmv1-tests-ext "$@" 2>&1 | tee "$TMPFILE" | while IFS= read -r line; do @@ -PASSED_FINAL=$(grep -E "(SUCCESS|FAILURE)! --" $TMPFILE | grep -o "[0-9]* Passed" | awk '{print $1}') -FAILED_FINAL=$(grep -E "(SUCCESS|FAILURE)! --" $TMPFILE | grep -o "[0-9]* Failed" | awk '{print $1}') -SKIPPED=$(grep -E "(SUCCESS|FAILURE)! --" $TMPFILE | grep -o "[0-9]* Skipped" | awk '{print $1}') -PENDING=$(grep -E "(SUCCESS|FAILURE)! --" $TMPFILE | grep -o "[0-9]* Pending" | awk '{print $1}') +PASSED_FINAL=$(grep -E "(SUCCESS|FAILURE)! --" "$TMPFILE" | grep -o "[0-9]* Passed" | awk '{print $1}') +FAILED_FINAL=$(grep -E "(SUCCESS|FAILURE)! --" "$TMPFILE" | grep -o "[0-9]* Failed" | awk '{print $1}') +SKIPPED=$(grep -E "(SUCCESS|FAILURE)! --" "$TMPFILE" | grep -o "[0-9]* Skipped" | awk '{print $1}') +PENDING=$(grep -E "(SUCCESS|FAILURE)! --" "$TMPFILE" | grep -o "[0-9]* Pending" | awk '{print $1}') @@ - grep '"result": "failed"' $TMPFILE -A 10 | while IFS= read -r line; do + grep '"result": "failed"' "$TMPFILE" -A 10 | while IFS= read -r line; do @@ - if ! grep -q '"result": "failed"' $TMPFILE; then + if ! grep -q '"result": "failed"' "$TMPFILE"; thenAlso applies to: 62-62, 162-165, 201-201, 215-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/hack/run-tests-clean.sh` at line 46, The trap and temp-file usages currently expand TMPFILE at trap setup and elsewhere without quoting, which breaks on spaces/special chars; change the trap to defer expansion (use single quotes: trap 'rm -f "$TMPFILE"' EXIT) and replace all unquoted expansions of TMPFILE (e.g., rm -f $TMPFILE, references to $TMPFILE) with quoted expansions ("$TMPFILE") so the variable is safely handled; ensure any other temp-file references in the script use "$TMPFILE" and not bare $TMPFILE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openshift/tests-extension/hack/run-tests-clean.sh`:
- Around line 201-212: The current parsing loop using grep -A 10 on $TMPFILE can
miss or misassociate failure details; replace the line-window heuristic with a
structured jq-based extraction: read $TMPFILE as JSON and for each item where
.result == "failed" output the .name and the .error (or nested fields) properly
unescaped and formatted, then remove the existing grep/... while loop and use a
jq command (targeting the same $TMPFILE variable) to reliably emit Test: <name>
and the error body so spacing or ordering changes in the JSON no longer break
parsing.
In `@openshift/tests-extension/Makefile`:
- Line 257: The Makefile target invoking the test wrapper passes the variable
SUITE unquoted; update the invocation of run-tests-clean.sh (the call that
currently uses run-suite $(SUITE)) to pass the variable quoted so the wrapper
receives a single safe argument (mirror how the test-single target passes
"$(TEST)"); modify the line invoking run-tests-clean.sh run-suite to use a
quoted $(SUITE) argument instead.
In `@openshift/tests-extension/README.md`:
- Around line 307-327: Remove the duplicate "Local Test using OLMv1 on
OpenShift" section in README.md by deleting the repeated heading and its steps
(the block that starts with "Local Test using OLMv1 on OpenShift" and includes
the Cluster Bot, KUBECONFIG mv/export, and running the olmv1-tests-ext binary);
keep the original earlier section and ensure only one copy of that heading and
its example commands remain to avoid doc drift.
- Line 168: Update the Markdown fenced code blocks that currently use plain
backticks so they include a language identifier (e.g., change ``` to ```text or
a more appropriate language) to satisfy markdown linting and consistent
rendering; search for the anonymous fenced code blocks in the README and add a
language tag to each fence to fix the lint warnings.
---
Nitpick comments:
In `@openshift/tests-extension/hack/run-tests-clean.sh`:
- Line 46: The trap and temp-file usages currently expand TMPFILE at trap setup
and elsewhere without quoting, which breaks on spaces/special chars; change the
trap to defer expansion (use single quotes: trap 'rm -f "$TMPFILE"' EXIT) and
replace all unquoted expansions of TMPFILE (e.g., rm -f $TMPFILE, references to
$TMPFILE) with quoted expansions ("$TMPFILE") so the variable is safely handled;
ensure any other temp-file references in the script use "$TMPFILE" and not bare
$TMPFILE.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c1cf9aa6-1413-4431-8e59-f828a3592db2
📒 Files selected for processing (3)
openshift/tests-extension/Makefileopenshift/tests-extension/README.mdopenshift/tests-extension/hack/run-tests-clean.sh
camilamacedo86
force-pushed
the
ote-improve-output
branch
from
62909ef to
b9eee40
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
openshift/tests-extension/devoutput/cmd/runsuitedev.go (2)
53-66: Simplify the signal switch statement.Both cases (
syscall.SIGINTanddefault) execute the same code (os.Exit(130)). The switch can be removed entirely.♻️ Proposed simplification
select { case sig := <-abortCh: fmt.Fprintf(os.Stderr, "Interrupted twice, exiting (%s)", sig) - switch sig { - case syscall.SIGINT: - os.Exit(130) - default: - os.Exit(130) - } + os.Exit(130) case <-time.After(30 * time.Minute):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/cmd/runsuitedev.go` around lines 53 - 66, The switch on the received signal in the abort handling block is redundant because both syscall.SIGINT and default call os.Exit(130); replace the entire switch in the select case that reads from abortCh with a single os.Exit(130) after the fmt.Fprintf call (look for abortCh, sig variable, and the switch block in runsuitedev.go) so the code simply prints the interrupt message and exits with 130.
103-105: Prefer returning an error over callingos.Exit(1)in RunE.Calling
os.Exit(1)insideRunEbypasses Cobra's error handling, prevents deferred functions from running (though in this casecancelCausewas already called), and makes the command harder to test. Return an error instead to let Cobra handle the exit.♻️ Proposed fix
if cleanWriter.HasFailures() { - os.Exit(1) + return fmt.Errorf("test failures occurred") } return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/cmd/runsuitedev.go` around lines 103 - 105, The check inside the command's RunE currently calls os.Exit(1) when cleanWriter.HasFailures() is true; change this to return an error instead so Cobra can handle termination and deferred cleanup. In the RunE implementation replace the os.Exit(1) call for the cleanWriter.HasFailures() branch with a returned error (e.g., return fmt.Errorf("clean writer detected failures") or a wrapped error), and ensure the function signature continues to return error so Cobra will set the exit code; reference the cleanWriter.HasFailures() check and the RunE function when making the change.openshift/tests-extension/devoutput/cmd/runtestdev.go (3)
60-73: Simplify the signal switch statement.Same issue as in
runsuitedev.go- both cases execute identical code.♻️ Proposed simplification
select { case sig := <-abortCh: fmt.Fprintf(os.Stderr, "Interrupted twice, exiting (%s)", sig) - switch sig { - case syscall.SIGINT: - os.Exit(130) - default: - os.Exit(130) - } + os.Exit(130) case <-time.After(30 * time.Minute):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/cmd/runtestdev.go` around lines 60 - 73, The select branch handling abortCh uses a switch on sig with identical branches; simplify by removing the switch and directly calling fmt.Fprintf(os.Stderr, "Interrupted twice, exiting (%s)", sig) followed by os.Exit(130) when receiving from abortCh (look for the abortCh select case and the sig variable and syscall.SIGINT reference in runtestdev.go) so the behavior remains the same but the redundant switch is eliminated.
111-113: Prefer returning an error over callingos.Exit(1)in RunE.Same issue as in
runsuitedev.go- callingos.Exit(1)bypasses Cobra's error handling.♻️ Proposed fix
if cleanWriter.HasFailures() { - os.Exit(1) + return fmt.Errorf("test failures occurred") } return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/cmd/runtestdev.go` around lines 111 - 113, Replace the direct process exit inside the command RunE handler with returning an error so Cobra can handle it: where you currently check cleanWriter.HasFailures() and call os.Exit(1), change that branch to return a descriptive error (e.g., return fmt.Errorf("tests failed") or errors.New("tests failed")) from the RunE implementation (the same change should be mirrored in RunSuiteDev's RunE in runsuitedev.go); ensure the RunE signature remains error-returning and remove the os.Exit call so Cobra's error handling and exit code propagation work correctly.
50-75: Consider extracting shared signal handling logic.The signal handling code (context setup, abort channel, signal goroutine) is duplicated between
runsuitedev.goandruntestdev.go. Consider extracting this to a shared helper function to reduce duplication and ensure consistent behavior.Example signature:
// In a shared file like signals.go func setupSignalHandling() (context.Context, func(error))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/cmd/runtestdev.go` around lines 50 - 75, The duplicated signal handling block inside the RunE anonymous function (context.WithCancelCause, abortCh, goroutine, signal.Notify) should be moved into a shared helper like setupSignalHandling() that returns (context.Context, func(error)) so both runtestdev.go and runsuitedev.go call it instead of duplicating logic; implement setupSignalHandling to create the cancelable context, start the abortCh goroutine (preserving the same cancelCause calls, stderr messages, exit codes and timeout behavior) and register signals via signal.Notify, then replace the inline block in RunE with a call to setupSignalHandling and defer the returned cancel function. Ensure the shared file imports context, os, os/signal, syscall, time and errors as needed.openshift/tests-extension/devoutput/README.md (2)
7-7: Add language identifier to fenced code block.Static analysis flagged this code block as missing a language specifier.
📝 Proposed fix
-``` +```text devoutput/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/README.md` at line 7, The README's fenced code block containing the line "devoutput/" is missing a language identifier; update that fence to include a language (e.g., change the opening "```" for the block that contains "devoutput/" to "```text") so static analysis recognizes the code block language.
31-31: Add language identifier to fenced code block.Static analysis flagged this code block as missing a language specifier.
📝 Proposed fix
-``` +```text Test Run → ResultWriter.Write(result) → Formatter → Colored Terminal Output🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/README.md` at line 31, The fenced code block containing "Test Run → ResultWriter.Write(result) → Formatter → Colored Terminal Output" is missing a language identifier; update the opening fence from ``` to a language-labeled fence (for example ```text) so the block reads ```text and the renderer/static analyzer recognizes the language.openshift/tests-extension/devoutput/pkg/output/writer.go (1)
12-17: Avoid retaining all results when only aggregated stats are used.
resultsis appended on every write but never consumed for output or exit logic. This adds avoidable memory growth for large suites.Refactor sketch
type CleanResultWriter struct { lock sync.Mutex formatter *Formatter - results []*et.ExtensionTestResult - writer io.Writer } @@ return &CleanResultWriter{ formatter: formatter, - results: make([]*et.ExtensionTestResult, 0), - writer: w, } } @@ - w.results = append(w.results, result) w.formatter.PrintTestStart(result.Name) w.formatter.PrintTestResult(result)Also applies to: 27-31, 44-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/pkg/output/writer.go` around lines 12 - 17, The CleanResultWriter is retaining every test result in the results []*et.ExtensionTestResult slice even though callers only need aggregated stats; remove the memory leak by deleting the results field and any appends to it (references to results in CleanResultWriter, and the code that appends in the Write/WriteResult method at the locations referenced), and instead update and persist only aggregate counters (e.g., pass/fail/total) on the CleanResultWriter or via Formatter; ensure any logic that previously relied on the slice reads the aggregate counters (or Formatter) and remove/replace code that clears or iterates results (lines referenced 27-31 and 44) so no unneeded slice growth occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openshift/tests-extension/devoutput/pkg/output/formatter.go`:
- Around line 2-4: Update the package comment in formatter.go to remove the
claim that this package "implements the OTE ResultWriter interface" and instead
describe its actual role as providing human-readable formatting utilities used
by the result writer; reference that the concrete implementation of the
interface is CleanResultWriter in writer.go so readers know where the interface
implementation lives and that formatter.go only contains formatting helpers for
colors, progress indicators, and summaries.
- Around line 85-105: The switch on result.Result (cases
et.ResultPassed/ResultFailed/ResultSkipped) lacks a default branch so unexpected
enum values leave counters unchanged; add a default case that treats unknown
results as failures to avoid under-reporting: increment f.failedCount, append a
FailedTest (use result.Name and set Error to include the unexpected result value
and any result.Error), and write a consistent formatted line to f.writer (e.g.,
mark as "UNKNOWN/FAILED" with ColorRed/ColorReset and the same Total summary
format) so the final summary correctly reflects the unexpected state.
In `@openshift/tests-extension/devoutput/pkg/output/writer.go`:
- Around line 40-46: Guard the Write method on CleanResultWriter against a nil
result: inside CleanResultWriter.Write, after acquiring the lock (w.lock.Lock())
check if the incoming result is nil and return early (optionally log/ignore) to
avoid dereferencing it; only append to w.results and call
w.formatter.PrintTestStart / w.formatter.PrintTestResult when result != nil.
Ensure the nil check is placed before any use of result (before append and
formatter calls) to prevent panics.
In `@openshift/tests-extension/devoutput/README.md`:
- Around line 7-16: Update the README directory listing to use the real
filenames instead of register.go: replace or augment the entry under cmd from
"register.go" to "register_dev.go" and "register_prod.go" (or note both files
exist) so developers can find register_dev.go and register_prod.go; keep the
rest of the tree (runsuitedev.go, runtestdev.go, pkg/output files) unchanged and
ensure the comment text still describes their purpose.
---
Nitpick comments:
In `@openshift/tests-extension/devoutput/cmd/runsuitedev.go`:
- Around line 53-66: The switch on the received signal in the abort handling
block is redundant because both syscall.SIGINT and default call os.Exit(130);
replace the entire switch in the select case that reads from abortCh with a
single os.Exit(130) after the fmt.Fprintf call (look for abortCh, sig variable,
and the switch block in runsuitedev.go) so the code simply prints the interrupt
message and exits with 130.
- Around line 103-105: The check inside the command's RunE currently calls
os.Exit(1) when cleanWriter.HasFailures() is true; change this to return an
error instead so Cobra can handle termination and deferred cleanup. In the RunE
implementation replace the os.Exit(1) call for the cleanWriter.HasFailures()
branch with a returned error (e.g., return fmt.Errorf("clean writer detected
failures") or a wrapped error), and ensure the function signature continues to
return error so Cobra will set the exit code; reference the
cleanWriter.HasFailures() check and the RunE function when making the change.
In `@openshift/tests-extension/devoutput/cmd/runtestdev.go`:
- Around line 60-73: The select branch handling abortCh uses a switch on sig
with identical branches; simplify by removing the switch and directly calling
fmt.Fprintf(os.Stderr, "Interrupted twice, exiting (%s)", sig) followed by
os.Exit(130) when receiving from abortCh (look for the abortCh select case and
the sig variable and syscall.SIGINT reference in runtestdev.go) so the behavior
remains the same but the redundant switch is eliminated.
- Around line 111-113: Replace the direct process exit inside the command RunE
handler with returning an error so Cobra can handle it: where you currently
check cleanWriter.HasFailures() and call os.Exit(1), change that branch to
return a descriptive error (e.g., return fmt.Errorf("tests failed") or
errors.New("tests failed")) from the RunE implementation (the same change should
be mirrored in RunSuiteDev's RunE in runsuitedev.go); ensure the RunE signature
remains error-returning and remove the os.Exit call so Cobra's error handling
and exit code propagation work correctly.
- Around line 50-75: The duplicated signal handling block inside the RunE
anonymous function (context.WithCancelCause, abortCh, goroutine, signal.Notify)
should be moved into a shared helper like setupSignalHandling() that returns
(context.Context, func(error)) so both runtestdev.go and runsuitedev.go call it
instead of duplicating logic; implement setupSignalHandling to create the
cancelable context, start the abortCh goroutine (preserving the same cancelCause
calls, stderr messages, exit codes and timeout behavior) and register signals
via signal.Notify, then replace the inline block in RunE with a call to
setupSignalHandling and defer the returned cancel function. Ensure the shared
file imports context, os, os/signal, syscall, time and errors as needed.
In `@openshift/tests-extension/devoutput/pkg/output/writer.go`:
- Around line 12-17: The CleanResultWriter is retaining every test result in the
results []*et.ExtensionTestResult slice even though callers only need aggregated
stats; remove the memory leak by deleting the results field and any appends to
it (references to results in CleanResultWriter, and the code that appends in the
Write/WriteResult method at the locations referenced), and instead update and
persist only aggregate counters (e.g., pass/fail/total) on the CleanResultWriter
or via Formatter; ensure any logic that previously relied on the slice reads the
aggregate counters (or Formatter) and remove/replace code that clears or
iterates results (lines referenced 27-31 and 44) so no unneeded slice growth
occurs.
In `@openshift/tests-extension/devoutput/README.md`:
- Line 7: The README's fenced code block containing the line "devoutput/" is
missing a language identifier; update that fence to include a language (e.g.,
change the opening "```" for the block that contains "devoutput/" to "```text")
so static analysis recognizes the code block language.
- Line 31: The fenced code block containing "Test Run →
ResultWriter.Write(result) → Formatter → Colored Terminal Output" is missing a
language identifier; update the opening fence from ``` to a language-labeled
fence (for example ```text) so the block reads ```text and the renderer/static
analyzer recognizes the language.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dff82b31-03df-4a23-a28a-34bea416443a
📒 Files selected for processing (10)
openshift/tests-extension/Makefileopenshift/tests-extension/README.mdopenshift/tests-extension/cmd/main.goopenshift/tests-extension/devoutput/README.mdopenshift/tests-extension/devoutput/cmd/register_dev.goopenshift/tests-extension/devoutput/cmd/register_prod.goopenshift/tests-extension/devoutput/cmd/runsuitedev.goopenshift/tests-extension/devoutput/cmd/runtestdev.goopenshift/tests-extension/devoutput/pkg/output/formatter.goopenshift/tests-extension/devoutput/pkg/output/writer.go
camilamacedo86
force-pushed
the
ote-improve-output
branch
from
b9eee40 to
54645d5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
openshift/tests-extension/devoutput/README.md (1)
7-17:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks.
Both fenced blocks are untyped; add
textto satisfy markdown lint and keep rendering consistent.📝 Proposed fix
-``` +```text devoutput/ ├── cmd/ # Cobra commands for local development │ ├── register_dev.go # Registers dev commands (with -tags dev) │ ├── register_prod.go # Returns nil (without -tags dev) │ ├── runsuitedev.go # run-suite-dev command │ └── runtestdev.go # run-test-dev command └── pkg/output/ # Output formatting ├── formatter.go # ANSI colors, progress, summaries └── writer.go # OTE ResultWriter implementation@@
-+text
Test Run → ResultWriter.Write(result) → Formatter → Colored Terminal OutputAlso applies to: 32-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/README.md` around lines 7 - 17, Update the fenced code blocks in openshift/tests-extension/devoutput/README.md to include the language identifier "text" so the markdown linter passes and rendering is consistent: add ```text to the directory tree block (the block showing devoutput/ with cmd/ and pkg/output/ and files like register_dev.go, formatter.go, writer.go) and also to the second fenced block that contains "Test Run → ResultWriter.Write(result) → Formatter → Colored Terminal Output" (also apply the same change to the block referenced at lines 32-34).openshift/tests-extension/Makefile (1)
262-262:⚠️ Potential issue | 🟡 MinorQuote
$(SUITE)intest-localinvocation.Unquoted variable expansion can split arguments unexpectedly. Quote it like
TESTis quoted intest-single.🛠️ Proposed fix
- @$(TOOLS_BIN_DIR)/olmv1-tests-ext run-suite-dev $(SUITE) + @$(TOOLS_BIN_DIR)/olmv1-tests-ext run-suite-dev "$(SUITE)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/Makefile` at line 262, The Makefile target invoking @$(TOOLS_BIN_DIR)/olmv1-tests-ext run-suite-dev $(SUITE) can split arguments if $(SUITE) contains spaces; change the invocation to quote the variable (use "$(SUITE)") so the suite name is passed as a single argument, mirroring how TEST is quoted in test-single and avoiding unexpected word-splitting.openshift/tests-extension/README.md (1)
154-167:⚠️ Potential issue | 🟡 MinorSpecify a language for the output example fences.
Add
textto both output snippets to satisfy markdown lint and keep docs consistent.📝 Proposed fix
-``` +```text [46/46] ▶ Running: [sig-olmv1][OCPFeatureGate:NewOLMWebhookProviderOpenshiftServiceCA] OLMv1 operator with webhooks should have a working validating webhook ✓ PASSED [194.1 seconds] (Total: ✓45 ✗0) @@ ✓ ALL TESTS PASSED!@@
-+text
Running Suite: - /Users/camilam/go/src/github/operator-framework-operator-controller/openshift/tests-extensionRandom Seed: 1753508546 - will randomize all specs
@@
}
]Also applies to: 189-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/README.md` around lines 154 - 167, The markdown code fences in openshift/tests-extension/README.md currently lack a language tag; update the two output example fences (the final test summary block starting with "[46/46] ▶ Running..." and the longer "Running Suite: ..." block referenced at lines 189-214) to use ```text instead of ``` so they satisfy the markdown linter and keep docs consistent.
🧹 Nitpick comments (1)
openshift/tests-extension/cmd/main.go (1)
312-327: Improve command identification and add safety guards for RunE wrapping.Using exact Use strings is fragile—if the format changes (multi-line layout, spacing), matching fails silently. Additionally,
originalRunEis called without nil-checking at line 327, which would panic if a command only definesRuninstead ofRunE.Use
command.Name()with a switch statement and guardRunEinvocation:Refactor
- // Identify run commands by their Use field - if command.Use == "run-suite NAME" || - command.Use == "run-test [-n NAME...] [NAME]" || - command.Use == "run-suite-dev NAME" || - command.Use == "run-test-dev [-n NAME...] [NAME]" { + switch command.Name() { + case "run-suite", "run-test", "run-suite-dev", "run-test-dev": // Save the original RunE function originalRunE := command.RunE + originalRun := command.Run // Wrap it with KUBECONFIG check command.RunE = func(cmd *cobra.Command, args []string) error { // Check KUBECONFIG before running the test if err := exutil.CheckKubeconfigSet(); err != nil { return err } // Call the original RunE function - return originalRunE(cmd, args) + if originalRunE != nil { + return originalRunE(cmd, args) + } + if originalRun != nil { + originalRun(cmd, args) + } + return nil } + command.Run = nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/cmd/main.go` around lines 312 - 327, The current identification using exact command.Use strings is brittle and you also call originalRunE without nil-checking which can panic; replace the Use checks with a switch on command.Name() matching "run-suite","run-test","run-suite-dev","run-test-dev", and wrap command.RunE by saving originalRunE := command.RunE; when invoking it, first check if originalRunE != nil then return originalRunE(cmd,args); else if command.Run != nil call command.Run(cmd,args) and convert its signature (if necessary) to return nil error; otherwise return a clear error (or cobra.ErrSubCommandRequired) so nil derefs are avoided. Ensure you still perform exutil.CheckKubeconfigSet() before calling the original handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openshift/tests-extension/devoutput/README.md`:
- Around line 7-17: Update the fenced code blocks in
openshift/tests-extension/devoutput/README.md to include the language identifier
"text" so the markdown linter passes and rendering is consistent: add ```text to
the directory tree block (the block showing devoutput/ with cmd/ and pkg/output/
and files like register_dev.go, formatter.go, writer.go) and also to the second
fenced block that contains "Test Run → ResultWriter.Write(result) → Formatter →
Colored Terminal Output" (also apply the same change to the block referenced at
lines 32-34).
In `@openshift/tests-extension/Makefile`:
- Line 262: The Makefile target invoking @$(TOOLS_BIN_DIR)/olmv1-tests-ext
run-suite-dev $(SUITE) can split arguments if $(SUITE) contains spaces; change
the invocation to quote the variable (use "$(SUITE)") so the suite name is
passed as a single argument, mirroring how TEST is quoted in test-single and
avoiding unexpected word-splitting.
In `@openshift/tests-extension/README.md`:
- Around line 154-167: The markdown code fences in
openshift/tests-extension/README.md currently lack a language tag; update the
two output example fences (the final test summary block starting with "[46/46] ▶
Running..." and the longer "Running Suite: ..." block referenced at lines
189-214) to use ```text instead of ``` so they satisfy the markdown linter and
keep docs consistent.
---
Nitpick comments:
In `@openshift/tests-extension/cmd/main.go`:
- Around line 312-327: The current identification using exact command.Use
strings is brittle and you also call originalRunE without nil-checking which can
panic; replace the Use checks with a switch on command.Name() matching
"run-suite","run-test","run-suite-dev","run-test-dev", and wrap command.RunE by
saving originalRunE := command.RunE; when invoking it, first check if
originalRunE != nil then return originalRunE(cmd,args); else if command.Run !=
nil call command.Run(cmd,args) and convert its signature (if necessary) to
return nil error; otherwise return a clear error (or
cobra.ErrSubCommandRequired) so nil derefs are avoided. Ensure you still perform
exutil.CheckKubeconfigSet() before calling the original handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fef8d52b-c063-4d52-954e-c93ed38699a4
📒 Files selected for processing (10)
openshift/tests-extension/Makefileopenshift/tests-extension/README.mdopenshift/tests-extension/cmd/main.goopenshift/tests-extension/devoutput/README.mdopenshift/tests-extension/devoutput/cmd/register_dev.goopenshift/tests-extension/devoutput/cmd/register_prod.goopenshift/tests-extension/devoutput/cmd/runsuitedev.goopenshift/tests-extension/devoutput/cmd/runtestdev.goopenshift/tests-extension/devoutput/pkg/output/formatter.goopenshift/tests-extension/devoutput/pkg/output/writer.go
✅ Files skipped from review due to trivial changes (1)
- openshift/tests-extension/devoutput/cmd/register_dev.go
🚧 Files skipped from review as they are similar to previous changes (4)
- openshift/tests-extension/devoutput/cmd/register_prod.go
- openshift/tests-extension/devoutput/pkg/output/writer.go
- openshift/tests-extension/devoutput/cmd/runtestdev.go
- openshift/tests-extension/devoutput/cmd/runsuitedev.go
camilamacedo86
force-pushed
the
ote-improve-output
branch
2 times, most recently
from
69e69bb to
84895bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
openshift/tests-extension/devoutput/pkg/output/writer.go (1)
12-17: Consider removing unused fields.The
resultsslice (line 15) is populated inWrite()but never read - only formatter methods are used. Similarly,writer(line 16) is stored but never accessed after construction (only passed to the formatter). If these are intended for future use, consider documenting the intent; otherwise, removing them would simplify the struct.♻️ Proposed simplification if fields are not needed
type CleanResultWriter struct { lock sync.Mutex formatter *Formatter - results []*et.ExtensionTestResult - writer io.Writer } func NewCleanResultWriter(w io.Writer) *CleanResultWriter { if w == nil { w = os.Stdout } formatter := NewFormatter(w) formatter.PrintHeader() return &CleanResultWriter{ formatter: formatter, - results: make([]*et.ExtensionTestResult, 0), - writer: w, } }And in
Write():- w.results = append(w.results, result) w.formatter.PrintTestStart(result.Name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/pkg/output/writer.go` around lines 12 - 17, The CleanResultWriter struct contains unused fields results and writer; remove these fields from the struct definition and update any constructors/initializers that set them, and remove any bookkeeping in Write() that appends to results; instead rely solely on the existing formatter methods (Formatter) and its use of the io.Writer passed to the formatter. Locate the CleanResultWriter type and its Write method and remove references to results and writer (and any sync of those fields), or if they are intended for future use, add a clear comment above the fields explaining their purpose; ensure all compilation references to results and writer are deleted or replaced so the code compiles.openshift/tests-extension/devoutput/pkg/output/formatter.go (1)
28-38: Remove unusedpendingCountfield.The
pendingCountfield (line 36) is declared but never incremented or displayed anywhere in the formatter. If pending tests are not tracked, this field should be removed for clarity.♻️ Proposed fix
type Formatter struct { writer io.Writer totalTests int currentTest int passedCount int failedCount int skippedCount int - pendingCount int failedTests []FailedTest }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/pkg/output/formatter.go` around lines 28 - 38, The Formatter struct contains an unused field pendingCount; remove the pendingCount field declaration from the Formatter type (the pendingCount symbol) and delete any initialization or references to it (if present elsewhere) so the struct only contains actually used counters (writer, totalTests, currentTest, passedCount, failedCount, skippedCount, failedTests). Ensure any code constructing or copying Formatter no longer expects pendingCount.openshift/tests-extension/devoutput/cmd/runtestdev.go (2)
64-69: Simplify redundant switch statement.Both cases exit with the same code (130), making the switch unnecessary.
♻️ Proposed simplification
case sig := <-abortCh: fmt.Fprintf(os.Stderr, "Interrupted twice, exiting (%s)", sig) - switch sig { - case syscall.SIGINT: - os.Exit(130) - default: - os.Exit(130) - } + os.Exit(130)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/cmd/runtestdev.go` around lines 64 - 69, The switch on sig (switch sig { case syscall.SIGINT: os.Exit(130) default: os.Exit(130) }) is redundant because both branches call os.Exit(130); simplify by removing the switch and calling os.Exit(130) directly (or, if intent is to handle specific signals differently later, replace with an if for syscall.SIGINT and a single os.Exit(130) fallback) so the code around sig, syscall.SIGINT, and os.Exit remains clear and minimal.
112-114: Consider returning an error instead of callingos.Exit(1)directly.Calling
os.Exit(1)bypasses anydeferstatements in the call stack and prevents Cobra from running its cleanup. Returning an error (e.g.,return fmt.Errorf("tests failed")) would be more idiomatic and allow the main function to control the exit code.However, if this is intentional to ensure a specific exit code for CI tooling, consider documenting this behavior.
♻️ Proposed change to return error instead
if cleanWriter.HasFailures() { - os.Exit(1) + return fmt.Errorf("one or more tests failed") } return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/cmd/runtestdev.go` around lines 112 - 114, The code currently calls os.Exit(1) when cleanWriter.HasFailures() which bypasses defers and Cobra cleanup; change this to return an error (for example return fmt.Errorf("tests failed")) from the enclosing function (e.g., the runtestdev command handler or function that contains the cleanWriter.HasFailures() check) so the caller/main can handle exit codes and cleanup; if you must keep a hard exit for CI, document that behavior adjacent to the cleanWriter.HasFailures() check.openshift/tests-extension/devoutput/cmd/runsuitedev.go (1)
48-68: Consider extracting duplicated signal handling into a shared helper.The signal handling logic (lines 48-69) is nearly identical to
runtestdev.go. Extracting this into a helper function would reduce duplication and make future changes easier.The same observations about the redundant switch statement (lines 57-62) and
os.Exit(1)pattern (lines 104-106) apply here as noted in theruntestdev.goreview.♻️ Example helper extraction
// In a shared file like helpers.go func setupSignalHandling() (context.Context, context.CancelCauseFunc) { ctx, cancelCause := context.WithCancelCause(context.Background()) abortCh := make(chan os.Signal, 2) go func() { <-abortCh fmt.Fprintf(os.Stderr, "Interrupted, terminating tests") cancelCause(errors.New("interrupt received")) select { case <-abortCh: fmt.Fprintf(os.Stderr, "Interrupted twice, exiting") os.Exit(130) case <-time.After(30 * time.Minute): fmt.Fprintf(os.Stderr, "Timed out during cleanup, exiting") os.Exit(130) } }() signal.Notify(abortCh, syscall.SIGINT, syscall.SIGTERM) return ctx, cancelCause }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/cmd/runsuitedev.go` around lines 48 - 68, Extract the duplicated signal handling in runsuitedev.go (the abortCh goroutine and cancelCause logic) into a shared helper like setupSignalHandling that returns (context.Context, context.CancelCauseFunc) and registers signals with signal.Notify; replace the inline abortCh creation and goroutine in runsuitedev.go and runtestdev.go with a call to that helper. While extracting, remove the redundant switch over sig (the case syscall.SIGINT / default both call os.Exit(130)) and unify exit codes/patterns (avoid inconsistent os.Exit(1) elsewhere) so the helper performs the single-exit behavior (e.g., os.Exit(130)) and the callers just use the returned ctx/cancelCause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openshift/tests-extension/cmd/main.go`:
- Around line 311-328: The closure assigned to command.RunE captures the loop
variable command incorrectly; fix it by shadowing or copying command into a
local variable inside the loop before creating the closure (e.g., localCmd :=
command) and reference that local variable inside the RunE closure instead of
command; keep capturing originalRunE as-is and ensure localCmd.Run and
localCmd.Name() are used along with exutil.CheckKubeconfigSet() so each command
gets its correct Run/Name at runtime.
---
Nitpick comments:
In `@openshift/tests-extension/devoutput/cmd/runsuitedev.go`:
- Around line 48-68: Extract the duplicated signal handling in runsuitedev.go
(the abortCh goroutine and cancelCause logic) into a shared helper like
setupSignalHandling that returns (context.Context, context.CancelCauseFunc) and
registers signals with signal.Notify; replace the inline abortCh creation and
goroutine in runsuitedev.go and runtestdev.go with a call to that helper. While
extracting, remove the redundant switch over sig (the case syscall.SIGINT /
default both call os.Exit(130)) and unify exit codes/patterns (avoid
inconsistent os.Exit(1) elsewhere) so the helper performs the single-exit
behavior (e.g., os.Exit(130)) and the callers just use the returned
ctx/cancelCause.
In `@openshift/tests-extension/devoutput/cmd/runtestdev.go`:
- Around line 64-69: The switch on sig (switch sig { case syscall.SIGINT:
os.Exit(130) default: os.Exit(130) }) is redundant because both branches call
os.Exit(130); simplify by removing the switch and calling os.Exit(130) directly
(or, if intent is to handle specific signals differently later, replace with an
if for syscall.SIGINT and a single os.Exit(130) fallback) so the code around
sig, syscall.SIGINT, and os.Exit remains clear and minimal.
- Around line 112-114: The code currently calls os.Exit(1) when
cleanWriter.HasFailures() which bypasses defers and Cobra cleanup; change this
to return an error (for example return fmt.Errorf("tests failed")) from the
enclosing function (e.g., the runtestdev command handler or function that
contains the cleanWriter.HasFailures() check) so the caller/main can handle exit
codes and cleanup; if you must keep a hard exit for CI, document that behavior
adjacent to the cleanWriter.HasFailures() check.
In `@openshift/tests-extension/devoutput/pkg/output/formatter.go`:
- Around line 28-38: The Formatter struct contains an unused field pendingCount;
remove the pendingCount field declaration from the Formatter type (the
pendingCount symbol) and delete any initialization or references to it (if
present elsewhere) so the struct only contains actually used counters (writer,
totalTests, currentTest, passedCount, failedCount, skippedCount, failedTests).
Ensure any code constructing or copying Formatter no longer expects
pendingCount.
In `@openshift/tests-extension/devoutput/pkg/output/writer.go`:
- Around line 12-17: The CleanResultWriter struct contains unused fields results
and writer; remove these fields from the struct definition and update any
constructors/initializers that set them, and remove any bookkeeping in Write()
that appends to results; instead rely solely on the existing formatter methods
(Formatter) and its use of the io.Writer passed to the formatter. Locate the
CleanResultWriter type and its Write method and remove references to results and
writer (and any sync of those fields), or if they are intended for future use,
add a clear comment above the fields explaining their purpose; ensure all
compilation references to results and writer are deleted or replaced so the code
compiles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 46c9a834-6310-4d47-8e69-786b011a66fa
📒 Files selected for processing (10)
openshift/tests-extension/Makefileopenshift/tests-extension/README.mdopenshift/tests-extension/cmd/main.goopenshift/tests-extension/devoutput/README.mdopenshift/tests-extension/devoutput/cmd/register_dev.goopenshift/tests-extension/devoutput/cmd/register_prod.goopenshift/tests-extension/devoutput/cmd/runsuitedev.goopenshift/tests-extension/devoutput/cmd/runtestdev.goopenshift/tests-extension/devoutput/pkg/output/formatter.goopenshift/tests-extension/devoutput/pkg/output/writer.go
✅ Files skipped from review due to trivial changes (3)
- openshift/tests-extension/devoutput/cmd/register_dev.go
- openshift/tests-extension/devoutput/cmd/register_prod.go
- openshift/tests-extension/devoutput/README.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openshift/tests-extension/cmd/main.go (1)
311-328:⚠️ Potential issue | 🔴 CriticalClosure still captures loop variable
commandincorrectly.The past review comment identified this issue, but it remains unfixed. The closure assigned to
command.RunEcaptures the loop variablecommand, so by execution time,command.Run(line 323) andcommand.Name()(line 327) will reference the last element ofallCommands, not the intended command.Looking at the relevant code snippets, all four commands (
run-suite,run-test,run-suite-dev,run-test-dev) only defineRunE, notRun. So thecommand.Runcheck is unreachable in practice, but thecommand.Name()error message would still be wrong.🐛 Proposed fix: shadow the loop variable
for _, command := range allCommands { + command := command // shadow loop variable for closure switch command.Name() { case "run-suite", "run-test", "run-suite-dev", "run-test-dev": originalRunE := command.RunE command.RunE = func(cmd *cobra.Command, args []string) error { if err := exutil.CheckKubeconfigSet(); err != nil { return err } if originalRunE != nil { return originalRunE(cmd, args) } - if command.Run != nil { - command.Run(cmd, args) + if cmd.Run != nil { + cmd.Run(cmd, args) return nil } - return fmt.Errorf("command %s has no Run or RunE function", command.Name()) + return fmt.Errorf("command %s has no Run or RunE function", cmd.Name()) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/cmd/main.go` around lines 311 - 328, The closure assigned to command.RunE captures the loop variable `command`, causing runtime references (e.g., `command.Run`, `command.Name()`) to point to the last element of `allCommands`; fix by shadowing the loop variable before assigning the closure (e.g., create a local copy like `c := command`) and use that local `c` inside the new RunE closure instead of `command`, and keep calling the preserved `originalRunE` and checking `c.Run`/`c.Name()` (and still call exutil.CheckKubeconfigSet()); this ensures `originalRunE`, the `Run` check, and the error message reference the correct command instance.
🧹 Nitpick comments (1)
openshift/tests-extension/devoutput/pkg/output/formatter.go (1)
29-38:pendingCountfield is unused and can be removed.The
pendingCountfield is defined but never incremented or read. Per the OTE framework types (context snippet 1), there are only three result states:ResultPassed,ResultSkipped, andResultFailed— no pending state exists. This field appears to be leftover scaffolding.♻️ Remove unused field
type Formatter struct { writer io.Writer totalTests int currentTest int passedCount int failedCount int skippedCount int - pendingCount int failedTests []FailedTest }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift/tests-extension/devoutput/pkg/output/formatter.go` around lines 29 - 38, The Formatter struct defines an unused pendingCount field; remove the pendingCount field from the Formatter struct declaration to eliminate dead state and align with OTE result states (only ResultPassed, ResultSkipped, ResultFailed). Update any constructors or tests that referenced Formatter.pendingCount (search for "pendingCount" to confirm none remain) and run tests to ensure no references remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openshift/tests-extension/devoutput/cmd/runsuitedev.go`:
- Around line 104-106: Replace the direct os.Exit(1) inside the RunE handler
with returning a sentinel error so deferred functions (like cancelCause) and
Cobra error handling still run: detect the failure via cleanWriter.HasFailures()
and return a named package-level error (e.g., ErrTestsFailed) from the RunE
function instead of calling os.Exit; update callers (e.g., main's root.Execute
handling) to check errors.Is(err, ErrTestsFailed) and call os.Exit(1) there.
Ensure the sentinel error is defined and documented in the same package as the
RunE implementation so callers can compare it.
---
Duplicate comments:
In `@openshift/tests-extension/cmd/main.go`:
- Around line 311-328: The closure assigned to command.RunE captures the loop
variable `command`, causing runtime references (e.g., `command.Run`,
`command.Name()`) to point to the last element of `allCommands`; fix by
shadowing the loop variable before assigning the closure (e.g., create a local
copy like `c := command`) and use that local `c` inside the new RunE closure
instead of `command`, and keep calling the preserved `originalRunE` and checking
`c.Run`/`c.Name()` (and still call exutil.CheckKubeconfigSet()); this ensures
`originalRunE`, the `Run` check, and the error message reference the correct
command instance.
---
Nitpick comments:
In `@openshift/tests-extension/devoutput/pkg/output/formatter.go`:
- Around line 29-38: The Formatter struct defines an unused pendingCount field;
remove the pendingCount field from the Formatter struct declaration to eliminate
dead state and align with OTE result states (only ResultPassed, ResultSkipped,
ResultFailed). Update any constructors or tests that referenced
Formatter.pendingCount (search for "pendingCount" to confirm none remain) and
run tests to ensure no references remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e23b4f40-f6d5-4f8b-bee3-0616a5a2036b
📒 Files selected for processing (10)
openshift/tests-extension/Makefileopenshift/tests-extension/README.mdopenshift/tests-extension/cmd/main.goopenshift/tests-extension/devoutput/README.mdopenshift/tests-extension/devoutput/cmd/register_dev.goopenshift/tests-extension/devoutput/cmd/register_prod.goopenshift/tests-extension/devoutput/cmd/runsuitedev.goopenshift/tests-extension/devoutput/cmd/runtestdev.goopenshift/tests-extension/devoutput/pkg/output/formatter.goopenshift/tests-extension/devoutput/pkg/output/writer.go
✅ Files skipped from review due to trivial changes (2)
- openshift/tests-extension/devoutput/cmd/register_dev.go
- openshift/tests-extension/devoutput/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- openshift/tests-extension/devoutput/cmd/register_prod.go
- openshift/tests-extension/devoutput/cmd/runtestdev.go
- openshift/tests-extension/devoutput/pkg/output/writer.go
camilamacedo86
force-pushed
the
ote-improve-output
branch
from
84895bc to
50c0c20
Compare
camilamacedo86
changed the title
|
@camilamacedo86: This pull request references OPRUN-4587 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
camilamacedo86
changed the title
camilamacedo86
force-pushed
the
ote-improve-output
branch
from
50c0c20 to
6ee5808
Compare
camilamacedo86
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
/retest-required |
camilamacedo86
force-pushed
the
ote-improve-output
branch
from
6ee5808 to
02035f6
Compare
camilamacedo86
force-pushed
the
ote-improve-output
branch
from
b39d22b to
510ce9a
Compare
camilamacedo86
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
|
@camilamacedo86: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Before this PR
He would have a huge JSON output, always hard to read locally as well, which is how we provide the result for the integrated systems, see below. That's still the same when we run
./bin/olmv1-tests-ext run-suite olmv1/allas it is executed by CI jobs. The change is only when we use the specific new commands (i.emake test-local SUITE=olmv1/all).Finishing like
After this PR
We can run the specific commands locally to have a human readble output:
Example:
make test-local SUITE=olmv1/allIn case of failures, we will have the details
Example
Generated-by: Claude