CONSOLE-5197: Improve Playwright artifact collection and add safety checks#16463
CONSOLE-5197: Improve Playwright artifact collection and add safety checks#16463vikram-raj wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vikram-raj: This pull request references CONSOLE-5197 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. |
|
@vikram-raj: GitHub didn't allow me to request PR reviews from the following users: openshift/team-ux-review. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
📝 WalkthroughWalkthroughThe PR establishes a robust artifact handling pipeline for Playwright end-to-end tests. It adds HTML report generation to the Playwright configuration, introduces a new helper function in the integration test script that validates the artifact directory, safely copies Playwright outputs (test results and HTML reports), and renames the JUnit XML appropriately. The prow wrapper script now validates that Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@frontend/integration-tests/test-playwright-e2e.sh`:
- Around line 94-97: The script currently accepts ARTIFACT_DIR="/" as valid
which is dangerous; update the validation around the case block handling
ARTIFACT_DIR to explicitly reject "/" (for example add a branch that checks
ARTIFACT_DIR = "/" and prints an error/returns non-zero before allowing absolute
paths, or add an explicit if [ "$ARTIFACT_DIR" = "/" ]; then echo "Error:
ARTIFACT_DIR must not be '/'" >&2; return 1; fi immediately after the case).
Apply the same explicit "/" rejection to the other validation sites referenced
(the other ARTIFACT_DIR checks at the locations noted).
- Around line 142-147: The copyArtifacts() helper currently preserves the
original test exit code unconditionally, so if copy_playwright_artifacts_to_dir
fails (e.g., invalid ARTIFACT_DIR) the script still exits 0; change
copyArtifacts() to capture the exit status of copy_playwright_artifacts_to_dir
(store its return value), and if that command fails (non-zero) exit with that
non-zero code (or a combined non-zero) instead of always exiting with the
original $exit_code; update references inside the copyArtifacts() function to
check the captured status of copy_playwright_artifacts_to_dir and use that for
the final exit when appropriate.
In `@test-prow-playwright-e2e.sh`:
- Around line 33-36: The current case allows ARTIFACT_DIR="/" which is unsafe;
update the guard around the ARTIFACT_DIR check (the case handling the
absolute-path check for ARTIFACT_DIR) to explicitly reject the root path: add a
check that if ARTIFACT_DIR is exactly "/" then print an error to stderr and exit
1 (same style/message as the other branch) — either by augmenting the existing
case arm for /* to perform this equality test and fail when equal to "/" or by
adding a separate conditional immediately after the case that tests [
"$ARTIFACT_DIR" = "/" ] and exits with an error.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 90367705-7b97-4726-9bb2-5facc27942ab
📒 Files selected for processing (3)
frontend/integration-tests/test-playwright-e2e.shfrontend/playwright.config.tstest-prow-playwright-e2e.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
frontend/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/**/*.{ts,tsx,js,jsx}: Never import from package index files (e.g.,@console/shared) in new code, as they can create circular dependencies and slow builds. Import from specific file paths instead.
Do not use backticks int()calls for i18n strings, as the i18n parser cannot extract keys from template literals. Use single or double quotes instead.
Files:
frontend/playwright.config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import from deprecated packages or use code with the
@deprecatedTSdoc tag in new code.
**/*.{ts,tsx}: Use React functional components with hooks instead of class components
State Management: Use React hooks and Context API (migrating away from legacy Redux/Immutable.js)
Hooks: Use existing hooks fromconsole-sharedwhen possible (useK8sWatchResource,useUserSettings, etc.)
API calls: Use k8s resource hooks for data fetching,consoleFetchJSONfor HTTP requests
Extensions: Use console extension points for plugin integration
Types: Check existing types inconsole-sharedbefore creating new ones
Dynamic Plugins: Use console extension points for plugin integration
Styling: Use SCSS modules co-located with components, PatternFly design system components, avoid any SCSS/CSS if possible
Accessibility: Follow WCAG 2.1 AA standards, use semantic HTML, ARIA labels where needed, ensure keyboard navigation, test with screen readers
i18n: UseuseTranslation('namespace')hook withkeyformat for translation keys
Error Handling: Use ErrorBoundary components and graceful degradation patterns
Optimize re-renders: UseuseCallbackfor memoized callbacks to avoid function recreation every render
Optimize re-renders: UseuseMemofor expensive computations to avoid recalculating on every render
Lazy loading: UseReact.lazy()to lazy load heavy components
TypeScript type safety: Avoid usinganytype; suggest proper type definitions and verify null/undefined are handled properly
Type component props properly: Reuse existing component prop types instead of duplicating type definitions
Use proper hooks: Use specialized hooks likeusePluginInfofor plugin data instead of generic data fetching patterns
Avoid deprecated components: Check for JSDoc@deprecatedtags, import paths containing/deprecated, andDEPRECATED_file name prefix before using components
Importing from barrel files and circular dependencies: Import directly from specific files instead...
Files:
frontend/playwright.config.ts
frontend/**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Never use absolute URLs or paths in the console code. The console runs behind a proxy under an arbitrary path.
Files:
frontend/playwright.config.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When writing code for static plugins, ensure that all
$codeRefreference the corresponding extension type from the@console/dynamic-plugin-sdkpackage.
Files:
frontend/playwright.config.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (TESTING.md)
**/*.{tsx,ts}: Always usepage.getByTestId('x')for Playwright selectors which queries[data-test="x"]. If a React element only has a legacy test attribute, adddata-testto the element. Never remove legacy attributes
Preferdata-testattributes in Cypress selectors (e.g.,cy.get('[data-test="create-deployment"]')) over brittle CSS/ARIA selectorsFile Naming: PascalCase for components, kebab-case for utilities,
*.spec.ts(x)for tests
Files:
frontend/playwright.config.ts
**/*.{go,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Use lowercase dash-separated names for all files to avoid git issues with case-insensitive file systems
Files:
frontend/playwright.config.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx,js,jsx}: New code MUST be written in TypeScript, not JavaScript
Prefer functional programming patterns and immutable data structures
Run the linter and follow all rules defined in .eslintrc
Never use absolute paths in code - the app should be able to run behind a proxy under an arbitrary path
Files:
frontend/playwright.config.ts
**/*.ts
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Plugin SDK Changes: Any updates to
console-dynamic-plugin-sdkshould aim to maintain backward compatibility as it's a public API - use theplugin-api-reviewskill to vet changes for public API impact and ensure proper documentation updates
Files:
frontend/playwright.config.ts
frontend/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (README.md)
frontend/**/*.{js,ts,tsx}: Support only the latest versions of Edge, Chrome, Safari, and Firefox browsers; IE 11 and earlier are not supported
CSP violations should be automatically reported to telemetry by parsing dynamic plugin names from securitypolicyviolation events, with throttling to prevent duplicate reports within a day
Files:
frontend/playwright.config.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (INTERNATIONALIZATION.md)
For dynamic translation keys that cannot be parsed by i18next-parser (t(key), t('key' + id), t(
key${id})), specify possible static values in comments for the parser to extract
Files:
frontend/playwright.config.ts
🔇 Additional comments (1)
frontend/playwright.config.ts (1)
58-58: LGTM!
- Validate ARTIFACT_DIR is set and is an absolute path before use - Add safety checks before rm -rf to prevent accidental deletion of wrong directories - Protect against misconfigured environment variables in CI These defensive improvements ensure that: 1. ARTIFACT_DIR is always validated as an absolute path 2. rm -rf operations only proceed if the target path is valid and not "/" 3. Functions return errors gracefully if validation fails Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
vikram-raj
force-pushed
the
playwright-artifact
branch
from
1a716b7 to
d05b024
Compare
|
/cc @stefanonardo |
|
@vikram-raj: 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. |
|
/lgtm |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@stefanonardo: This PR has been marked as verified by 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. |
|
/label docs-approved |
openshift-ci
Bot
added
docs-approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, stefanonardo, vikram-raj 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
4 participants
Analysis / Root cause:
The Playwright E2E test infrastructure needed improvements in artifact collection for CI environments:
rm -rfoperationsSolution description:
This PR implements comprehensive artifact collection and safety improvements:
Enhanced Artifact Collection (commit 9a99fd0):
$ARTIFACT_DIR/junit-playwright.xmlfor CI reporting$ARTIFACT_DIR/playwright-reportfor debuggingcp -ato preserve file attributesSafety Improvements (commit 1a716b7):
rm -rfoperations to prevent accidental deletionScreenshots / screen recording:
N/A - Infrastructure/CI changes only
Test setup:
Set up Playwright E2E test environment:
cd frontend yarn install npx playwright installTest cases:
ARTIFACT_DIR=/tmp/test-artifacts ./integration-tests/test-playwright-e2e.shand verify artifacts are copied$ARTIFACT_DIR/junit-playwright.xml$ARTIFACT_DIR/playwright-report/index.htmlbash -n)Browser conformance:
N/A - Infrastructure changes only
Additional info:
Summary by CodeRabbit