[WIP] OCPBUGS-82510: Fix web-terminal-adminuser e2e tests broken by createRoot#16284
[WIP] OCPBUGS-82510: Fix web-terminal-adminuser e2e tests broken by createRoot#16284stefanonardo wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@stefanonardo: This pull request references Jira Issue OCPBUGS-82510, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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
bot
added
the
kind/cypress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stefanonardo 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 |
stefanonardo
force-pushed
the
OCPBUGS-82510-clean
branch
from
5fd0f89 to
338adc7
Compare
📝 WalkthroughWalkthroughThis pull request refactors integration tests across the Web Terminal plugin and navigation components. Changes include: (1) adding scroll-into-view logic before perspective-switcher visibility assertions to improve test reliability, (2) standardizing DOM element selectors from CSS class-based selectors to data-test attributes for loading state checks, (3) simplifying terminal initialization logic by removing conditional re-initialization paths and their associated imports and control flow, and (4) consolidating terminal workspace cleanup steps within the Gherkin feature background rather than repeating them across individual scenarios. The modifications maintain existing test coverage while reducing brittle conditional branching in test step definitions. 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 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.
🧹 Nitpick comments (1)
frontend/packages/integration-tests/views/nav.ts (1)
13-15: Optional: centralize repeated toggle visibility prep into one helper.The same
byLegacyTestID(...).scrollIntoView().should('be.visible')chain is repeated three times; extracting it would make future selector/wait tuning easier.♻️ Suggested refactor
+const ensurePerspectiveSwitcherToggleVisible = () => + cy.byLegacyTestID('perspective-switcher-toggle').scrollIntoView().should('be.visible'); ... - cy.byLegacyTestID('perspective-switcher-toggle') - .scrollIntoView() - .should('be.visible') + ensurePerspectiveSwitcherToggleVisible() .then(($toggle) => { ... - cy.byLegacyTestID('perspective-switcher-toggle') - .scrollIntoView() - .should('be.visible') + ensurePerspectiveSwitcherToggleVisible() .then(($toggle) => { ... - cy.byLegacyTestID('perspective-switcher-toggle') - .scrollIntoView() - .should('be.visible') + ensurePerspectiveSwitcherToggleVisible() .find('.pf-v6-c-menu-toggle__text')Also applies to: 35-37, 80-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests/views/nav.ts` around lines 13 - 15, Extract the repeated chain cy.byLegacyTestID(...).scrollIntoView().should('be.visible') into a small helper (e.g., ensureVisibleToggle or ensureTestIdVisible) and call that helper wherever the pattern appears (references: cy.byLegacyTestID and the 'perspective-switcher-toggle' selector and the other similar selectors currently using the same chain); update the three occurrences that use that chain to call the new helper with the appropriate test id so future tuning of scroll/wait behavior is centralized and easy to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/packages/integration-tests/views/nav.ts`:
- Around line 13-15: Extract the repeated chain
cy.byLegacyTestID(...).scrollIntoView().should('be.visible') into a small helper
(e.g., ensureVisibleToggle or ensureTestIdVisible) and call that helper wherever
the pattern appears (references: cy.byLegacyTestID and the
'perspective-switcher-toggle' selector and the other similar selectors currently
using the same chain); update the three occurrences that use that chain to call
the new helper with the appropriate test id so future tuning of scroll/wait
behavior is centralized and easy to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8410ebc6-f6c9-434c-822c-39d6d7637a26
📒 Files selected for processing (5)
frontend/packages/integration-tests/views/nav.tsfrontend/packages/webterminal-plugin/integration-tests/features/web-terminal/web-terminal-adminuser.featurefrontend/packages/webterminal-plugin/integration-tests/support/step-definitions/common/webTerminal.tsfrontend/packages/webterminal-plugin/integration-tests/support/step-definitions/pages/web-terminal/initTerminal-page.tsfrontend/packages/webterminal-plugin/integration-tests/support/step-definitions/web-terminal/web-terminal-adminuser.ts
📜 Review details
🔇 Additional comments (6)
frontend/packages/integration-tests/views/nav.ts (1)
13-15: Good reliability improvement for perspective-switcher interactions.Adding
scrollIntoView()before the visibility check is a solid way to reduce CI flakiness when sidebar/favorites layout shifts push the toggle out of viewport.Also applies to: 35-37, 80-82
frontend/packages/webterminal-plugin/integration-tests/support/step-definitions/web-terminal/web-terminal-adminuser.ts (1)
45-45: Good selector fix and synchronization point.Using
[data-test="loading-box"]aligns this step with the actual loading component and avoids the previous no-op selector behavior.frontend/packages/webterminal-plugin/integration-tests/support/step-definitions/common/webTerminal.ts (1)
51-51: Consistent loading selector update here as well.This keeps the common step aligned with the same loading-state strategy used across the web terminal e2e flow.
frontend/packages/webterminal-plugin/integration-tests/support/step-definitions/pages/web-terminal/initTerminal-page.ts (2)
3-3: Import cleanup looks correct.
appremains actively used, and the simplified import surface matches the refactored flow.
21-21: CreateRoot-safe wait in start flow.Waiting on
[data-test="loading-box-body"]after Start is a cleaner, retry-friendly replacement for the prior synchronous branch logic.frontend/packages/webterminal-plugin/integration-tests/features/web-terminal/web-terminal-adminuser.feature (1)
9-9: Strong test-isolation improvement in Background.Running workspace cleanup before each scenario is the right move to prevent cross-test contamination and flaky retries.
|
/retest |
1 similar comment
|
/retest |
stefanonardo
force-pushed
the
OCPBUGS-82510-clean
branch
2 times, most recently
from
5d94cbb to
f699061
Compare
|
/retest |
1 similar comment
|
/retest |
stefanonardo
force-pushed
the
OCPBUGS-82510-clean
branch
from
f699061 to
e86feb2
Compare
|
/retest |
1 similar comment
|
/retest |
Replace clickStartButton's synchronous $body.find() check with a Cypress retry-based assertion. createRoot defers renders, so the old check always failed and triggered a recovery path that broke the test state. Also fix broken cos-status-box selectors, move workspace cleanup to Background, and add scrollIntoView() to the perspective switcher toggle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
stefanonardo
force-pushed
the
OCPBUGS-82510-clean
branch
from
e86feb2 to
5fa579d
Compare
stefanonardo
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/retest |
|
@stefanonardo: 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. |
3 participants
Summary
The
web-terminal-adminuser.featuree2e tests were disabled after the createRoot migration (CONSOLE-4512) because they started failing consistently. Root cause analysis (via CI Cypress Cloud replays and local reproduction) revealed:clickStartButtonininitTerminal-page.tsused a synchronous$body.find()jQuery check to detect a loading indicator. Under legacyReactDOM.render, React committed updates synchronously after a click, so the indicator was in the DOM when the check ran. UndercreateRoot, renders are deferred — the check always fails and triggers an OCPBUGS-44891 recovery path that leaves the app in a broken state (drawer open, sidebar scrolled, workspace already created), causing cascading failures on retry.cos-status-box cos-status-box--loadingselectors matched nonexistent HTML elements (should be CSS class selectors), passing immediately as no-ops.Changes
initTerminal-page.ts: Replace synchronous$body.find()+ recovery path with a Cypress retry-basedcy.get().should('exist')that waits for the loading indicatorweb-terminal-adminuser.ts,webTerminal.ts: Replace brokencos-status-boxselectors with[data-test="loading-box"]web-terminal-adminuser.feature: Move workspace cleanup to Background (runs before each test), remove disabling commentnav.ts: AddscrollIntoView()to perspective switcher toggle — on CI, sidebar favorites from earlier specs can push it out of viewTest plan
web-terminal-adminuser.featuretests pass on CI (e2e-gcp-console job)🤖 Generated with Claude Code
Summary by CodeRabbit