OCPBUGS-84230: [release-4.22] - Re-enable cluster-settings/update-modal.cy.ts e2e test disabled for createRoot adoption#16319
Conversation
…reateRoot adoption
openshift-ci-robot
added
the
jira/valid-reference
|
@cajieh: This pull request references Jira Issue OCPBUGS-84230, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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-robot
added
the
jira/valid-bug
|
@cajieh: This pull request references Jira Issue OCPBUGS-84230, which is valid. 3 validation(s) were run on this bug
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 APPROVED This pull-request has been approved by: cajieh 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
📝 WalkthroughWalkthroughThis changeset establishes infrastructure for testing the Cluster Settings update modal with MachineConfigPool state transitions. It introduces test fixtures representing paused and unpaused worker pool configurations, a WebSocket stub to intercept MCP list watch requests and prevent Redux state overwrites during integration tests, and an expanded test suite that exercises modal behavior across different MCP states. Test utilities are enhanced with optional visit parameters and defensive assertions to ensure UI element readiness before interactions, alongside a more graceful MCP pause command that tolerates environments lacking the Machine Config Operator API. 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 (5)
frontend/packages/integration-tests/tests/cluster-settings/update-modal.cy.ts (3)
42-46: Consider aliasing MCP intercept for deterministic waits.The
clusterVersionintercept is aliased and waited on, but the MCP list intercept isn't. While the WebSocket stub prevents race conditions with watch updates, if MCP data arrival timing matters for UI state, an explicit wait could improve reliability:cy.intercept('GET', MCP_LIST_PATTERN, { body: machineConfigPoolListWithPausedWorker, - }); + }).as('mcpList'); visitClusterSettingsWithMcpStub(); cy.wait(`@${CLUSTER_VERSION_ALIAS}`, WAIT_OPTIONS); + cy.wait('@mcpList');This is optional if current reliability is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests/tests/cluster-settings/update-modal.cy.ts` around lines 42 - 46, Add an alias to the MCP intercept and wait for it to ensure deterministic test timing: when stubbing the GET for MCP_LIST_PATTERN with machineConfigPoolListWithPausedWorker (the existing cy.intercept call), chain or assign an alias like '@mcpList' and then call cy.wait('@mcpList') before calling visitClusterSettingsWithMcpStub() or before the subsequent cy.wait for CLUSTER_VERSION_ALIAS so the test explicitly waits for the MCP response arrival and stabilizes UI state.
53-73: Clarify intent ofpauseMCP()calls when MCP data is fully stubbed.The test intercepts MCP list responses with fixture data, yet still calls
clusterSettings.pauseMCP()which patches the real cluster'smcp/worker. Since the UI reads from the stubbed response (not live cluster state), andpauseMCPtolerates API absence withfailOnNonZeroExit: false, these calls appear to have no effect on test behavior.If
pauseMCP()is needed for clusters without the stub taking effect (unlikely given the intercept), consider adding a comment explaining the intent. Otherwise, removing these calls would simplify the test:it('changes based on the cluster', () => { cy.log('with a paused Worker MCP'); - clusterSettings.pauseMCP(); clusterSettings.openUpdateModalAndOpenDropdown(); // ... - clusterSettings.pauseMCP('false');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests/tests/cluster-settings/update-modal.cy.ts` around lines 53 - 73, The test calls clusterSettings.pauseMCP() and clusterSettings.pauseMCP('false') even though the MCP list is fully stubbed by fixtures and the UI reads the stubbed data; either remove these calls to avoid confusing no-op side effects, or add a one-line comment next to the clusterSettings.pauseMCP() calls explaining that they are a defensive fallback for non-stubbed environments (and that pauseMCP tolerates missing APIs via failOnNonZeroExit: false). Locate the calls by the clusterSettings.pauseMCP identifier in update-modal.cy.ts and apply one of the two options consistently.
93-110: Track the CI-only flakiness for future resolution.The
isLocalDevEnvironmentguard is a pragmatic workaround, but this flaky section should be tracked for investigation. Consider adding a comment with the relevant JIRA issue or creating a follow-up ticket to stabilize this for CI.The underlying interaction pattern (toggle → switch → menu item selection) may have timing issues in CI that could be addressed with additional synchronization or PatternFly dropdown stabilization helpers.
Would you like me to open an issue to track investigating this CI flakiness, or suggest additional synchronization patterns that might help?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests/tests/cluster-settings/update-modal.cy.ts` around lines 93 - 110, This flaky CI guard around isLocalDevEnvironment hides a test that needs tracking and stabilization; add a concise TODO comment above the if (isLocalDevEnvironment) block referencing a new JIRA/issue (create one if none exists) that states the flakiness and the failing interaction sequence (toggle → switch → menu item for data-test ids update-cluster-modal, dropdown-with-switch-toggle, dropdown-with-switch-switch, dropdown-with-switch-menu-item-4.16.4), then create the issue with steps-to-reproduce and label it "e2e-flaky"; also in the test replace the fragile clicks with explicit synchronization (e.g., assert visibility/interactive state before click using cy.byTestID(...).should('be.visible').should('not.be.disabled') or add retries/wait-for-menu-open helpers) and reference the new issue ID in the TODO comment.frontend/packages/integration-tests/views/cluster-settings.ts (1)
6-11: Pragmatic tolerance for absent MCO API.The
failOnNonZeroExit: falsewith explanatory comment is appropriate. This allows the test suite to run on non-OCP clusters or CI profiles where the Machine Config Operator isn't available.However, silently ignoring failures means the test might pass even if the patch was intended but failed unexpectedly. Consider logging the exec result for debugging:
cy.exec( `oc patch mcp/worker --patch '{ "spec": { "paused": ${value} } }' --type=merge`, { failOnNonZeroExit: false }, ).then((result) => { if (result.code !== 0) { cy.log(`pauseMCP skipped: ${result.stderr}`); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests/views/cluster-settings.ts` around lines 6 - 11, The pauseMCP helper currently swallows oc patch failures via { failOnNonZeroExit: false } — update pauseMCP to append a .then handler to the cy.exec call (referencing the pauseMCP function) that inspects the returned result and calls cy.log with a clear message and result.stderr (or result.stdout) when result.code !== 0 so failures are visible for debugging while still tolerating missing MCO API.frontend/packages/integration-tests/support/stub-machine-config-pool-watch-ws.ts (1)
9-13: Consider tightening URL pattern to match list watches only.The comment says "MCP list watch URL only," but the substring check
machineconfiguration.openshift.io/v1/machineconfigpoolsalso matches individual resource watches (e.g.,.../machineconfigpools/worker). The test file'sMCP_LIST_PATTERNuses?*to target list requests specifically.If individual MCP resource watches are used elsewhere in the Console, this broader match could interfere. For current tests this likely works fine, but for precision you could check for a trailing
?or lack of a resource name:if (urlString.includes('machineconfigpools?') || urlString.endsWith('machineconfigpools')) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests/support/stub-machine-config-pool-watch-ws.ts` around lines 9 - 13, The current check if (urlString.includes('machineconfiguration.openshift.io/v1/machineconfigpools')) is too broad and matches individual MCP resource watches; tighten the condition in the stub in stub-machine-config-pool-watch-ws.ts to only match list/watch URLs by checking the query/list form (e.g., test for 'machineconfigpools?' in the URL or urlString.endsWith('machineconfigpools')) or use the existing MCP_LIST_PATTERN used in tests; update the if that returns the stub WebSocket (the block returning { close: () => {} }) to use this stricter check so only MCP list/watch requests are intercepted.
🤖 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/support/stub-machine-config-pool-watch-ws.ts`:
- Around line 9-13: The current check if
(urlString.includes('machineconfiguration.openshift.io/v1/machineconfigpools'))
is too broad and matches individual MCP resource watches; tighten the condition
in the stub in stub-machine-config-pool-watch-ws.ts to only match list/watch
URLs by checking the query/list form (e.g., test for 'machineconfigpools?' in
the URL or urlString.endsWith('machineconfigpools')) or use the existing
MCP_LIST_PATTERN used in tests; update the if that returns the stub WebSocket
(the block returning { close: () => {} }) to use this stricter check so only MCP
list/watch requests are intercepted.
In
`@frontend/packages/integration-tests/tests/cluster-settings/update-modal.cy.ts`:
- Around line 42-46: Add an alias to the MCP intercept and wait for it to ensure
deterministic test timing: when stubbing the GET for MCP_LIST_PATTERN with
machineConfigPoolListWithPausedWorker (the existing cy.intercept call), chain or
assign an alias like '@mcpList' and then call cy.wait('@mcpList') before calling
visitClusterSettingsWithMcpStub() or before the subsequent cy.wait for
CLUSTER_VERSION_ALIAS so the test explicitly waits for the MCP response arrival
and stabilizes UI state.
- Around line 53-73: The test calls clusterSettings.pauseMCP() and
clusterSettings.pauseMCP('false') even though the MCP list is fully stubbed by
fixtures and the UI reads the stubbed data; either remove these calls to avoid
confusing no-op side effects, or add a one-line comment next to the
clusterSettings.pauseMCP() calls explaining that they are a defensive fallback
for non-stubbed environments (and that pauseMCP tolerates missing APIs via
failOnNonZeroExit: false). Locate the calls by the clusterSettings.pauseMCP
identifier in update-modal.cy.ts and apply one of the two options consistently.
- Around line 93-110: This flaky CI guard around isLocalDevEnvironment hides a
test that needs tracking and stabilization; add a concise TODO comment above the
if (isLocalDevEnvironment) block referencing a new JIRA/issue (create one if
none exists) that states the flakiness and the failing interaction sequence
(toggle → switch → menu item for data-test ids update-cluster-modal,
dropdown-with-switch-toggle, dropdown-with-switch-switch,
dropdown-with-switch-menu-item-4.16.4), then create the issue with
steps-to-reproduce and label it "e2e-flaky"; also in the test replace the
fragile clicks with explicit synchronization (e.g., assert
visibility/interactive state before click using
cy.byTestID(...).should('be.visible').should('not.be.disabled') or add
retries/wait-for-menu-open helpers) and reference the new issue ID in the TODO
comment.
In `@frontend/packages/integration-tests/views/cluster-settings.ts`:
- Around line 6-11: The pauseMCP helper currently swallows oc patch failures via
{ failOnNonZeroExit: false } — update pauseMCP to append a .then handler to the
cy.exec call (referencing the pauseMCP function) that inspects the returned
result and calls cy.log with a clear message and result.stderr (or
result.stdout) when result.code !== 0 so failures are visible for debugging
while still tolerating missing MCO API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: fbb93c38-d7ed-46c2-9745-5966da88465f
📒 Files selected for processing (4)
frontend/packages/integration-tests/mocks/machine-config-pool.tsfrontend/packages/integration-tests/support/stub-machine-config-pool-watch-ws.tsfrontend/packages/integration-tests/tests/cluster-settings/update-modal.cy.tsfrontend/packages/integration-tests/views/cluster-settings.ts
📜 Review details
🔇 Additional comments (5)
frontend/packages/integration-tests/mocks/machine-config-pool.ts (1)
1-66: Well-structured minimal fixtures for MCP test scenarios.Clean approach using
structuredCloneto avoid mutation side effects. The fixtures are appropriately minimal, containing only the fields the UI actually reads (as documented in the comment). The differentiated timestamps for master vs worker ensure stable sort order in tests.One consideration: if any downstream code or future tests rely on
metadata.uidfor resource identity (e.g., reconciliation logic), you may need to add unique UIDs to these fixtures. For current scope, this looks sufficient.frontend/packages/integration-tests/support/stub-machine-config-pool-watch-ws.ts (1)
35-41: Clean fallback to native WebSocket for non-MCP URLs.Good that other WebSocket connections remain functional. The
eslint-disable-next-line no-param-reassignis appropriate here since mutatingwin.WebSocketis intentional for the test stub.Note: there's no restore mechanism, but since Cypress isolates test runs and page reloads reset the window context, this is acceptable for the test infrastructure.
frontend/packages/integration-tests/tests/cluster-settings/update-modal.cy.ts (1)
24-29: Solid reload helper with WebSocket stub re-installation.Good pattern: the
onBeforeLoadcallback ensures the WebSocket stub is re-applied on page reload, and thehorizontal-link-Detailsassertion confirms the page has loaded before proceeding.frontend/packages/integration-tests/views/cluster-settings.ts (2)
2-5: Good enhancement to supportvisitOptionspassthrough.Clean extension of the helper to accept optional
Cypress.VisitOptions, enabling callers to injectonBeforeLoadfor WebSocket stubbing without duplicating visit logic.
13-29: Thorough synchronization for modal/dropdown interactions.The enhanced assertions address timing issues from createRoot adoption:
- Visibility and enabled checks prevent premature interaction
aria-busycheck respects PatternFly loading states- Re-querying before click handles potential DOM replacement
The pattern is slightly verbose with the duplicate query, but this defensive approach is justified for stabilizing previously flaky tests. The tradeoff of extra DOM queries for reliability is acceptable in test code.
|
@cajieh: 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. |
2 participants
Manual Backport:
Original PR
The pull request title must be prefixed with a Jira issue in order to be merged. For example:
OCPBUGS-OCPBUGS-84230 <title>
Analysis / Root cause:
See Original PR
Solution description:
See Original PR
Screenshots / screen recording:
See Original PR
Test setup:
Test cases:
See Original PR
Additional info:
See the original PR
Reviewers and assignees:
Console Approver:
/assign @sg00dwin
-->
Summary by CodeRabbit