OCPBUGS-82512: Fix knative e2e test failures caused by createRoot timing#16289
OCPBUGS-82512: Fix knative e2e test failures caused by createRoot timing#16289sg00dwin wants to merge 1 commit intoopenshift:mainfrom
Conversation
The createRoot migration defers React state batching, causing the namespace bar dropdown and topology node labels to take longer to appear in the DOM. Add explicit timeouts to clickProjectDropdown() and getNode() so Cypress retries until the elements are present, and re-enable the knative-ci.feature test suite. Assisted by Claude
openshift-ci-robot
added
jira/valid-reference
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-82512, 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sg00dwin 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 |
openshift-ci
bot
added
component/dev-console
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-82512, 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. |
📝 WalkthroughWalkthroughThis pull request strengthens integration test reliability across three frontend modules by introducing explicit timeout configurations. The dev-console test page now includes a 30-second wait assertion before clicking the namespace dropdown, ensuring element existence before interaction. The topology page retrieves nodes with an explicit 30-second Cypress timeout parameter. Additionally, the knative-plugin test feature removes a prior disabled status annotation, enabling the previously flagged test scenario. These changes collectively address timing issues in the test harness without altering feature logic or exposed APIs. 🚥 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/topology/integration-tests/support/pages/topology/topology-page.ts`:
- Around line 292-295: The current chain uses a 30s timeout on
cy.get(topologyPO.graph.nodeLabel) but not on the subsequent text match, so
change the text match to use the explicit timeout; update the call that
currently does .contains(nodeName) to .contains(nodeName, { timeout: 30000 })
(or alternatively use cy.contains(topologyPO.graph.nodeLabel, nodeName, {
timeout: 30000 })) so the node label text matching shares the same 30s window.
🪄 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: Pro Plus
Run ID: 94d13d23-3aaf-40fa-91a0-8a9cd81c459e
📒 Files selected for processing (3)
frontend/packages/dev-console/integration-tests/support/pages/app.tsfrontend/packages/knative-plugin/integration-tests/features/e2e/knative-ci.featurefrontend/packages/topology/integration-tests/support/pages/topology/topology-page.ts
📜 Review details
🔇 Additional comments (2)
frontend/packages/knative-plugin/integration-tests/features/e2e/knative-ci.feature (1)
1-1: Tag update correctly re-enables this feature in CI.Removing
@to-dohere is consistent with the configured Cypress tag filter that excludes@to-do, so this suite will be picked up again under@knative+@smoke.frontend/packages/dev-console/integration-tests/support/pages/app.ts (1)
253-257: Good stabilization of dropdown readiness before interaction.The explicit 30s wait and existence check on Line 253-Line 254 are a solid fix for delayed namespace-bar rendering under concurrent React timing.
| return cy | ||
| .get(topologyPO.graph.nodeLabel, { timeout: 30000 }) | ||
| .should('be.visible') | ||
| .contains(nodeName); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Cypress, for cy.get(selector, { timeout: 30000 }).contains(text), does .contains(text)inherit the 30s timeout fromget, or does it use defaultCommandTimeout unless timeout is explicitly passed?
💡 Result:
No, .contains(text) does not inherit the 30s timeout from cy.get(selector, { timeout: 30000 }). It uses defaultCommandTimeout unless a timeout is explicitly passed to .contains. Both cy.get and .contains are queries that retry until they resolve or timeout. The timeout option set on cy.get applies specifically to waiting for the selector elements to appear in the DOM (up to 30s). Once cy.get resolves (yields elements), .contains then searches within those yielded elements for the text. Since .contains has no explicit timeout, it uses the default defaultCommandTimeout (typically 4s) for its own retry logic—retrying the text search within the subject until found or that default timeout expires. This is evidenced by official Cypress documentation: .contains explicitly states its timeout defaults to defaultCommandTimeout [6,7,10]. Query chains retry from the top using the initiating command's timeout for the entire chain's assertions, but each query command (like .contains) has its own timeout for its resolution [11,12,24]. Examples consistently show setting timeout on the first query affects chained assertions, but child queries like .contains require their own timeout option for extended waiting [16].
Citations:
- 1: https://docs.cypress.io/api/commands/get.html
- 2: https://docs.cypress.io/api/commands/get
- 3: https://docs.cypress.io/api/commands/contains.htm
- 4: https://docs.cypress.io/api/commands/contains.html
- 5: https://docs.cypress.io/api/commands/contains
- 6: https://docs.cypress.io/guides/core-concepts/retry-ability.html
- 7: https://docs.cypress.io/guides/core-concepts/retry-ability
- 8: https://stackoverflow.com/questions/56779120/cypress-custom-timeout-in-specific-should-command
- 9: https://docs.cypress.io/guides/core-concepts/introduction-to-Cypress
🏁 Script executed:
#!/bin/bash
# Verify the file exists and check the actual code at the specified lines
if [ -f "frontend/packages/topology/integration-tests/support/pages/topology/topology-page.ts" ]; then
echo "=== File exists, showing context around lines 292-295 ==="
sed -n '288,300p' "frontend/packages/topology/integration-tests/support/pages/topology/topology-page.ts" | cat -n
else
echo "File not found. Searching for topology-page.ts..."
find . -name "topology-page.ts" -type f
fiRepository: openshift/console
Length of output: 675
🌐 Web query:
Cypress cy.contains(selector, text, options) - Does this syntax exist and does the timeout option work on it?
💡 Result:
Yes, the syntax Cypress cy.contains(selector, text, options) exists, and the timeout option works on it. The official Cypress documentation explicitly lists this syntax and confirms that the options object includes a 'timeout' property, which overrides the defaultCommandTimeout to specify how long to wait for cy.contains to resolve before timing out.
Citations:
- 1: https://docs.cypress.io/api/commands/contains
- 2: https://docs.cypress.io/api/commands/contains.html
- 3: https://docs.cypress.io/api/commands/contains.htm
Timeout on text match must be explicit to prevent flakiness.
The 30-second timeout on cy.get() applies only to finding label elements; once any label appears, contains(nodeName) runs with the default 4-second timeout for text matching. This leaves a window where the correct node name may not be found before timeout, defeating the purpose of the 30-second window.
Apply the timeout directly to the text-matching operation:
Proposed fix
return cy
- .get(topologyPO.graph.nodeLabel, { timeout: 30000 })
- .should('be.visible')
- .contains(nodeName);
+ .contains(topologyPO.graph.nodeLabel, nodeName, { timeout: 30000 })
+ .should('be.visible');📝 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.
| return cy | |
| .get(topologyPO.graph.nodeLabel, { timeout: 30000 }) | |
| .should('be.visible') | |
| .contains(nodeName); | |
| return cy | |
| .contains(topologyPO.graph.nodeLabel, nodeName, { timeout: 30000 }) | |
| .should('be.visible'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/topology/integration-tests/support/pages/topology/topology-page.ts`
around lines 292 - 295, The current chain uses a 30s timeout on
cy.get(topologyPO.graph.nodeLabel) but not on the subsequent text match, so
change the text match to use the explicit timeout; update the call that
currently does .contains(nodeName) to .contains(nodeName, { timeout: 30000 })
(or alternatively use cy.contains(topologyPO.graph.nodeLabel, nodeName, {
timeout: 30000 })) so the node label text matching shares the same 30s window.
|
Agree with coderabbit's comment, besides that lgtm. The failing backend ci seems just a flake. @sg00dwin |
|
/retest-required |
|
@sg00dwin: The following test failed, say
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
The createRoot migration defers React state batching, causing the
namespace bar dropdown and topology node labels to take longer to
appear in the DOM. Add explicit timeouts to clickProjectDropdown()
and getNode() so Cypress retries until the elements are present,
and re-enable the knative-ci.feature test suite.
Summary
.should('exist')toclickProjectDropdown()inapp.ts— fixes 7 of 8 failing knative tests where the namespace bardropdown wasn't found in time under concurrent rendering
getNode()intopology-page.ts— fixes 1 of 8failing knative tests where the topology SVG node label wasn't found in
time
knative-ci.featuretest suite by removing@to-dotagadded in c18637a
Details
The
createRootmigration (CONSOLE-4512, #16202) switched React intoconcurrent rendering mode, which defers state update batching. The
NamespaceBarDropdownscomponent returnsnulluntil theCAN_LIST_NSfeature flag resolves — under
ReactDOM.render()this was synchronous,but under
createRootthe resolution is deferred past Cypress's default4s timeout. The topology node label has the same timing issue with async
SVG rendering.
No production code is changed. The timeouts are ceilings, not delays —
Cypress resolves immediately when the element is present.
Follows the same pattern established in 60ac10e and 8f85029.
Test plan
add-page.featurelocally viayarn test-cypress-dev-console— all tests that exercise
clickProjectDropdown()pass under concurrentrendering
pull-ci-openshift-console-main-e2e-gcp-consolevalidatesknative-ci.feature end-to-end
Assisted by Claude
Summary by CodeRabbit