OLS-2654 - adding artifact exporting. Should help investigations#1940
OLS-2654 - adding artifact exporting. Should help investigations#1940JoaoFula wants to merge 3 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR extends Cypress test infrastructure with OpenShift artifact collection. It adds a Node-side artifact gathering function that exports cluster resources and pod logs, registers it as a Cypress task, invokes it in test teardown, and improves test reliability by handling specific uncaught exceptions and simplifying tour dismissal logic. ChangesOpenShift Artifact Collection for Cypress Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kyoto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypress.config.ts (1)
148-161:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftArtifacts will be overwritten on every spec — investigation value is mostly lost.
gatherClusterArtifactsis invoked fromafter:specand always writes to the same path (./gui_test_screenshots/artifacts/cluster). With N specs, each resource YAML and each pod log is rewritten N times and only the artifacts from the last spec survive — exactly the opposite of what you want when investigating a failure in an earlier spec. It also runs unconditionally on passing specs, paying the fullocround-trip cost (and up to a 30 s timeout per failed call) on every spec.Two reasonable fixes (either alone is fine):
- Move collection to
after:runso it runs once at the end of the suite.- Keep it in
after:specbut namespace the output by spec (and optionally only when the spec failed), e.g.path.join(ARTIFACTS_DIR, 'cluster', sanitize(spec.relative)), so you retain per-spec context.♻️ Sketch: per-spec dir, only on failure
- on('after:spec', (spec: Cypress.Spec, results: CypressCommandLine.RunResult) => { + on('after:spec', (spec: Cypress.Spec, results: CypressCommandLine.RunResult) => { if (results && results.video) { // Do we have failures for any retry attempts? const failures = results.tests.some((test) => test.attempts.some((attempt) => attempt.state === 'failed'), ); if (!failures && fs.existsSync(results.video)) { // Delete the video if the spec passed and no tests retried fs.unlinkSync(results.video); } + if (failures) { + const specKey = spec.relative.replace(/[^a-zA-Z0-9._-]+/g, '_'); + console.log('Gathering cluster artifacts...'); + gatherClusterArtifacts(config.env.KUBECONFIG_PATH || '', specKey); + } + } else { + // still collect once if there is no video signal + console.log('Gathering cluster artifacts...'); + gatherClusterArtifacts(config.env.KUBECONFIG_PATH || '', spec.relative.replace(/[^a-zA-Z0-9._-]+/g, '_')); } - console.log('Gathering cluster artifacts...'); - gatherClusterArtifacts(config.env.KUBECONFIG_PATH || ''); });And in
gatherClusterArtifacts, append the spec key toclusterDir/podLogsDir.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypress.config.ts` around lines 148 - 161, The current after:spec handler calls gatherClusterArtifacts for every spec and writes to a shared ./gui_test_screenshots/artifacts/cluster path which causes artifacts to be overwritten and wastes time; either (A) move the gatherClusterArtifacts call from the 'after:spec' hook into the 'after:run' hook so collection runs once after the whole suite, or (B) keep it in the 'after:spec' hook but change the invocation to only run on spec failures (check results.tests for failed attempts) and pass a namespaced output directory (e.g. join ARTIFACTS_DIR, 'cluster', sanitize(spec.relative) or use spec.relative as a subdir) so gatherClusterArtifacts writes per-spec folders instead of overwriting the shared path; update gatherClusterArtifacts to accept and use that per-spec path and stop unconditionally calling it with config.env.KUBECONFIG_PATH.
🧹 Nitpick comments (1)
cypress.config.ts (1)
56-68: ⚡ Quick winPod log gathering misses init/ephemeral containers and previous-instance logs.
For investigation, the most interesting logs are usually the ones from a crashed container (previous instance) and from init containers that failed before the main container started. The current loop only iterates
pod.spec.containersand callsoc logsfor the current instance, so:
- Crash-looping containers' last fatal output is not captured (need
--previous).initContainersandephemeralContainerslogs are skipped entirely.♻️ Proposed change
- const containers = (pod.spec?.containers || []).map((c: { name: string }) => c.name); - for (const container of containers) { - const logs = runOC( - ['logs', `pod/${podName}`, '-c', container, '-n', OLS_NAMESPACE], - kubeconfigPath, - ); - if (logs) { - fs.writeFileSync(path.join(podLogsDir, `${podName}-${container}.log`), logs); - } - } + const containers = [ + ...((pod.spec?.initContainers || []) as { name: string }[]), + ...((pod.spec?.containers || []) as { name: string }[]), + ...((pod.spec?.ephemeralContainers || []) as { name: string }[]), + ].map((c) => c.name); + for (const container of containers) { + for (const previous of [false, true]) { + const args = ['logs', `pod/${podName}`, '-c', container, '-n', OLS_NAMESPACE]; + if (previous) args.push('--previous'); + const logs = runOC(args, kubeconfigPath); + if (logs) { + const suffix = previous ? '.previous.log' : '.log'; + fs.writeFileSync(path.join(podLogsDir, `${podName}-${container}${suffix}`), logs); + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypress.config.ts` around lines 56 - 68, The current pod log loop only reads pod.spec.containers for the current instance and misses init/ephemeral containers and previous-instance logs; update the loop that uses pods.items, pod.metadata?.name and runOC to also iterate pod.spec?.initContainers and pod.spec?.ephemeralContainers (fallback to empty arrays) and call runOC twice per container: once for the current logs and once with the '--previous' flag to capture the last crashed instance; ensure you create distinct filenames under podLogsDir (e.g., include "init"/"ephemeral" and "previous" markers in the filename) and guard on podName/container existence before calling runOC using the existing OLS_NAMESPACE constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cypress.config.ts`:
- Around line 148-161: The current after:spec handler calls
gatherClusterArtifacts for every spec and writes to a shared
./gui_test_screenshots/artifacts/cluster path which causes artifacts to be
overwritten and wastes time; either (A) move the gatherClusterArtifacts call
from the 'after:spec' hook into the 'after:run' hook so collection runs once
after the whole suite, or (B) keep it in the 'after:spec' hook but change the
invocation to only run on spec failures (check results.tests for failed
attempts) and pass a namespaced output directory (e.g. join ARTIFACTS_DIR,
'cluster', sanitize(spec.relative) or use spec.relative as a subdir) so
gatherClusterArtifacts writes per-spec folders instead of overwriting the shared
path; update gatherClusterArtifacts to accept and use that per-spec path and
stop unconditionally calling it with config.env.KUBECONFIG_PATH.
---
Nitpick comments:
In `@cypress.config.ts`:
- Around line 56-68: The current pod log loop only reads pod.spec.containers for
the current instance and misses init/ephemeral containers and previous-instance
logs; update the loop that uses pods.items, pod.metadata?.name and runOC to also
iterate pod.spec?.initContainers and pod.spec?.ephemeralContainers (fallback to
empty arrays) and call runOC twice per container: once for the current logs and
once with the '--previous' flag to capture the last crashed instance; ensure you
create distinct filenames under podLogsDir (e.g., include "init"/"ephemeral" and
"previous" markers in the filename) and guard on podName/container existence
before calling runOC using the existing OLS_NAMESPACE constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 905cbc1a-f7fe-4062-afac-d3df6eea7076
📒 Files selected for processing (1)
cypress.config.ts
JoaoFula
force-pushed
the
adding-artifact-exporting-4.19
branch
from
c605708 to
54b07a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cypress/support/e2e.js`:
- Around line 7-15: Update the Cypress.on('uncaught:exception', ...) handler to
narrow suppression by requiring the plugin marker to appear in the error stack
as well as the message, and emit a log when you suppress it; specifically, in
the callback for uncaught:exception check both
err.message?.includes('__load_plugin_entry__') and
err.stack?.includes('__load_plugin_entry__'), and when both match call
Cypress.log (or console.warn) with a short message and include the err object
before returning false to suppress.
In `@tests/tests/lightspeed-install.cy.ts`:
- Line 334: The unconditional click on the tour-step footer
(cy.byTestID('tour-step-footer-secondary', { timeout: MINUTE }).click()) causes
flakiness when the guided tour is not rendered; make the click conditional by
checking for the element's presence first and only performing the click when the
element exists (use cy.byTestID(...) with a .then / length check or equivalent
existence check and call click() only when present) so the step in
tests/tests/lightspeed-install.cy.ts no longer fails when the tour is pre-marked
completed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ec0cd17-9874-4cf6-badd-fc38f9189f27
📒 Files selected for processing (3)
cypress.config.tscypress/support/e2e.jstests/tests/lightspeed-install.cy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress.config.ts
| Cypress.on('uncaught:exception', (err) => { | ||
| // The console defines __load_plugin_entry__ via webpack module federation | ||
| // before loading plugin bundles, but race conditions during page load can | ||
| // cause the plugin script to execute before the global is set. | ||
| // This is a console-internal timing issue, not a test failure. | ||
| if (err.message?.includes('__load_plugin_entry__')) { | ||
| return false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Cypress, what are the recommended best practices for Cypress.on('uncaught:exception') handlers so specific known errors are suppressed without masking unrelated test failures?
💡 Result:
The recommended best practices for Cypress.on('uncaught:exception') handlers, as per official documentation, focus on selectively suppressing known errors while allowing unrelated failures to propagate [1][2]. 1. Use conditional logic based on error.message to ignore only specific known errors. Return false only for matching errors; otherwise, let the test fail naturally [1]. Example for global handler (e.g., in cypress/support/e2e.js): Cypress.on('uncaught:exception', (err, runnable) => { if (err.message.includes('list not defined')) { // known 3rd-party error return false; } // Acts as no-op for other errors, failing the test }); [1][3][4] 2. Prefer cy.on inside specific tests for scoped handling, avoiding global suppression. This automatically unbinds after the test [1][3]. Example: it('handles known error', () => { cy.on('uncaught:exception', (e, runnable) => { if (e.message.includes('Things went bad')) { return false; } }); // test code that triggers the error }); [1][3] 3. For one-time suppression, use cy.once [5]. 4. Avoid blanket return false, as it masks all errors [1][6]. Log errors for debugging [3]. 5. Place global handlers in support files; test-specific in 'it' blocks [1][4]. Note: Recent Cypress versions (e.g., 13+) may wrap app errors; check err.message for app details or err.name/err.cause [7][8]. Fix root causes in the app when possible [2].
Citations:
- 1: https://docs.cypress.io/api/cypress-api/catalog-of-events
- 2: https://docs.cypress.io/app/references/error-messages
- 3: https://stackoverflow.com/questions/68001586/cypress-on-uncaught-exception-is-not-working
- 4: https://stackoverflow.com/questions/53845493/cypress-uncaught-error-despite-cy-onuncaughtexception
- 5: https://stackoverflow.com/questions/74210257/turn-on-cypress-uncaughtexception-after-turning-it-off
- 6: https://mohamedsaidibrahim.medium.com/effective-handling-of-errors-and-exceptions-in-cypress-best-practices-8201df710462
- 7: Issue with uncaught:exception Handling in Cypress 13.12.0 cypress-io/cypress#29797
- 8: Unable to handle uncaught:exception within test cypress-io/cypress#29142
Narrow the exception filter to add specificity and logging.
At line 12, suppressing exceptions by message substring alone is a valid but minimal approach. While message-only filtering is standard in Cypress examples for known errors, adding a stack trace check makes the filter more specific and reduces the risk of accidentally masking unrelated failures. Additionally, logging when suppression occurs is an explicit best practice for debugging.
Proposed patch
Cypress.on('uncaught:exception', (err) => {
- if (err.message?.includes('__load_plugin_entry__')) {
+ const isPluginBootstrapRace =
+ err.message?.includes('__load_plugin_entry__') &&
+ /lightspeed-console-plugin|module federation|webpack/i.test(err.stack ?? '');
+
+ if (isPluginBootstrapRace) {
+ Cypress.log({
+ name: 'uncaught:exception:suppressed',
+ message: err.message,
+ });
return false;
}
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cypress/support/e2e.js` around lines 7 - 15, Update the
Cypress.on('uncaught:exception', ...) handler to narrow suppression by requiring
the plugin marker to appear in the error stack as well as the message, and emit
a log when you suppress it; specifically, in the callback for uncaught:exception
check both err.message?.includes('__load_plugin_entry__') and
err.stack?.includes('__load_plugin_entry__'), and when both match call
Cypress.log (or console.warn) with a short message and include the err object
before returning false to suppress.
f323f37 to
7081e43
Compare
|
Let's split the skip tour fix into a separate PR to keeps things separate / atomic |
JoaoFula
force-pushed
the
adding-artifact-exporting-4.19
branch
from
b141525 to
71a7573
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cypress.config.ts`:
- Around line 146-148: The current gatherClusterArtifacts wrapper calls
gatherClusterArtifacts(config.env.KUBECONFIG_PATH || '') which passes an empty
string and lets oc use the ambient kubeconfig; change it to require a non-empty
config.env.KUBECONFIG_PATH: check config.env.KUBECONFIG_PATH inside the
gatherClusterArtifacts wrapper and if it's falsy either skip collection by
returning null (and log a warning) or fail fast by throwing a clear error; only
call the inner gatherClusterArtifacts(path) when config.env.KUBECONFIG_PATH is
present to avoid using the default cluster context.
- Around line 155-158: The variable failures can be undefined when results.tests
is nullish, causing !failures to be true and triggering video deletion
unintentionally; update the assignment of failures (the const named failures
that uses results.tests?.some(...)) to default unknown outcomes to "failures
exist" (true) using the nullish coalescing operator so the subsequent check if
(!failures && fs.existsSync(results.video)) only deletes videos when we know
tests passed and not when test results are unknown.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: badcdf48-dece-4ca2-99fb-cfd1d57f6997
📒 Files selected for processing (3)
.tekton/integration-tests/lightspeed-console-pre-commit.yamlcypress.config.tstests/tests/lightspeed-install.cy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .tekton/integration-tests/lightspeed-console-pre-commit.yaml
| gatherClusterArtifacts() { | ||
| gatherClusterArtifacts(config.env.KUBECONFIG_PATH || ''); | ||
| return null; |
There was a problem hiding this comment.
Avoid collecting artifacts from an unintended cluster context.
At Line 147, falling back to '' means oc can use ambient/default kubeconfig, which may export data from the wrong cluster. Skip collection (or fail fast) when KUBECONFIG_PATH is not set.
Proposed patch
gatherClusterArtifacts() {
- gatherClusterArtifacts(config.env.KUBECONFIG_PATH || '');
+ const kubeconfigPath = config.env.KUBECONFIG_PATH;
+ if (!kubeconfigPath) {
+ console.warn('Skipping cluster artifact gathering: KUBECONFIG_PATH is not set');
+ return null;
+ }
+ gatherClusterArtifacts(kubeconfigPath);
return null;
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gatherClusterArtifacts() { | |
| gatherClusterArtifacts(config.env.KUBECONFIG_PATH || ''); | |
| return null; | |
| gatherClusterArtifacts() { | |
| const kubeconfigPath = config.env.KUBECONFIG_PATH; | |
| if (!kubeconfigPath) { | |
| console.warn('Skipping cluster artifact gathering: KUBECONFIG_PATH is not set'); | |
| return null; | |
| } | |
| gatherClusterArtifacts(kubeconfigPath); | |
| return null; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cypress.config.ts` around lines 146 - 148, The current gatherClusterArtifacts
wrapper calls gatherClusterArtifacts(config.env.KUBECONFIG_PATH || '') which
passes an empty string and lets oc use the ambient kubeconfig; change it to
require a non-empty config.env.KUBECONFIG_PATH: check config.env.KUBECONFIG_PATH
inside the gatherClusterArtifacts wrapper and if it's falsy either skip
collection by returning null (and log a warning) or fail fast by throwing a
clear error; only call the inner gatherClusterArtifacts(path) when
config.env.KUBECONFIG_PATH is present to avoid using the default cluster
context.
| const failures = results.tests?.some((test) => | ||
| test.attempts?.some((attempt) => attempt.state === 'failed'), | ||
| ); | ||
| if (!failures && fs.existsSync(results.video)) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "cypress.config.ts" | head -5Repository: openshift/lightspeed-console
Length of output: 90
🏁 Script executed:
cat -n cypress.config.ts | sed -n '150,165p'Repository: openshift/lightspeed-console
Length of output: 790
Default to "keep video" when test outcome is unknown to prevent unintended deletion of failure videos.
At line 155-157, results.tests?.some(...) can return undefined when results.tests is null or undefined. Since !undefined evaluates to true, the video deletion logic executes unintentionally in these cases. Update the assignment to use the nullish coalescing operator to treat unknown outcomes as "failures exist":
Proposed patch
- const failures = results.tests?.some((test) =>
+ const failures = results.tests?.some((test) =>
test.attempts?.some((attempt) => attempt.state === 'failed'),
- );
- if (!failures && fs.existsSync(results.video)) {
+ ) ?? true;
+ if (!failures && fs.existsSync(results.video)) {
// Delete the video if the spec passed and no tests retried
fs.unlinkSync(results.video);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const failures = results.tests?.some((test) => | |
| test.attempts?.some((attempt) => attempt.state === 'failed'), | |
| ); | |
| if (!failures && fs.existsSync(results.video)) { | |
| const failures = results.tests?.some((test) => | |
| test.attempts?.some((attempt) => attempt.state === 'failed'), | |
| ) ?? true; | |
| if (!failures && fs.existsSync(results.video)) { | |
| // Delete the video if the spec passed and no tests retried | |
| fs.unlinkSync(results.video); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cypress.config.ts` around lines 155 - 158, The variable failures can be
undefined when results.tests is nullish, causing !failures to be true and
triggering video deletion unintentionally; update the assignment of failures
(the const named failures that uses results.tests?.some(...)) to default unknown
outcomes to "failures exist" (true) using the nullish coalescing operator so the
subsequent check if (!failures && fs.existsSync(results.video)) only deletes
videos when we know tests passed and not when test results are unknown.
reverting changes around tour
adding ls
JoaoFula
force-pushed
the
adding-artifact-exporting-4.19
branch
2 times, most recently
from
2d5f488 to
db11d22
Compare
changing the artifact gathering to reverting tour changes reverting tour changes reverting tour changes
JoaoFula
force-pushed
the
adding-artifact-exporting-4.19
branch
from
db11d22 to
ecd03a4
Compare
2 participants
Summary by CodeRabbit
Tests
Chores