Improve Cypress tests PF4 branch#1532
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
web/cypress/views/netflow-page.ts (1)
410-410: 💤 Low valueConsider removing
{ force: true }option.Using
{ force: true }can mask real UI issues like elements being covered or disabled. Lines 421 and 427 don't require it. If the Overview tab needs forcing due to being pre-selected, consider adding a comment explaining why.🤖 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 `@web/cypress/views/netflow-page.ts` at line 410, The test is using cy.get('#tabs-container').contains('Overview').click({ force: true }) which can hide real UI issues; remove the { force: true } option so the click fails if the tab is covered/disabled, and only reintroduce force with a short inline comment explaining why (e.g., pre-selected state) if you have a justified reason; update the call in netflow-page.ts to plain .click() and ensure the test asserts the tab is visible/enabled before clicking (or add the explanatory comment next to the forced click if you must keep it).web/cypress/integration-tests/quickFilters.cy.ts (1)
35-39: ⚡ Quick winReplace fixed sleep with condition-based waits.
Line 35 introduces a hard-coded delay, which can still flake under slow/fast cluster conditions. Use
oc wait --for=create+oc wait --for=condition=Readyinstead of sleeping.Suggested diff
- // Wait for pods to be created - cy.wait(10000) - - // Wait for pods to be ready + // Wait for pods to exist, then be ready + cy.adminCLI('oc wait --for=create pod -l app=nginx -n test-server-56222 --timeout=120s') + cy.adminCLI('oc wait --for=create pod/client -n test-client-56222 --timeout=120s') cy.adminCLI('oc wait --for=condition=Ready pod -l app=nginx -n test-server-56222 --timeout=120s') - cy.adminCLI('oc wait --for=condition=Ready pod -n test-client-56222 client --timeout=120s') + cy.adminCLI('oc wait --for=condition=Ready pod/client -n test-client-56222 --timeout=120s')🤖 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 `@web/cypress/integration-tests/quickFilters.cy.ts` around lines 35 - 39, Replace the hard-coded cy.wait(10000) in quickFilters.cy.ts with condition-based waits: remove the sleep and use cy.adminCLI to run oc wait --for=created pod -l app=nginx -n test-server-56222 --timeout=120s (and similarly for the client pod) followed by oc wait --for=condition=Ready pod -l app=nginx -n test-server-56222 --timeout=120s and oc wait --for=condition=Ready pod -n test-client-56222 client --timeout=120s so the test waits for pod creation and readiness instead of a fixed delay.
🤖 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 `@web/cypress/views/netobserv.ts`:
- Around line 88-97: The oc get csv call in install() and visitFlowcollector()
can pick non-Succeeded CSVs; change the cy.adminCLI query to filter for
status.phase==Succeeded (e.g., oc get csv -n openshift-netobserv-operator -o
custom-columns=":metadata.name" --field-selector status.phase=Succeeded or use
--selector/jsonpath to only return Succeeded CSV names) so you only pick CSVs in
Succeeded state, factor the query+parsing into a shared helper (e.g.,
getSucceededNetObservCsvName()) used by both install() and visitFlowcollector(),
and replace the result typing from any to a proper CLI response interface
(typing the result as { stdout?: string; stderr?: string }) so there are no any
annotations.
- Around line 89-90: Replace the use of the any type for the CLI result in the
Promise .then callbacks by typing the parameter as { stdout?: string } instead
of any; update both occurrences where .then((result: any) => { ... }) (the first
callback around the stdout trim and the second one near lines 116–117) to
.then((result: { stdout?: string }) => { ... }) so the code continues to extract
const stdout = result.stdout ? result.stdout.trim() : '' with a proper
TypeScript type.
---
Nitpick comments:
In `@web/cypress/integration-tests/quickFilters.cy.ts`:
- Around line 35-39: Replace the hard-coded cy.wait(10000) in quickFilters.cy.ts
with condition-based waits: remove the sleep and use cy.adminCLI to run oc wait
--for=created pod -l app=nginx -n test-server-56222 --timeout=120s (and
similarly for the client pod) followed by oc wait --for=condition=Ready pod -l
app=nginx -n test-server-56222 --timeout=120s and oc wait --for=condition=Ready
pod -n test-client-56222 client --timeout=120s so the test waits for pod
creation and readiness instead of a fixed delay.
In `@web/cypress/views/netflow-page.ts`:
- Line 410: The test is using
cy.get('#tabs-container').contains('Overview').click({ force: true }) which can
hide real UI issues; remove the { force: true } option so the click fails if the
tab is covered/disabled, and only reintroduce force with a short inline comment
explaining why (e.g., pre-selected state) if you have a justified reason; update
the call in netflow-page.ts to plain .click() and ensure the test asserts the
tab is visible/enabled before clicking (or add the explanatory comment next to
the forced click if you must keep it).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: decc3c9b-50d7-42e7-a758-7098bc480adf
📒 Files selected for processing (10)
web/cypress/fixtures/test-server-client.yamlweb/cypress/integration-tests/dns_tracking.cy.tsweb/cypress/integration-tests/netflow_cluster_admin_group.cy.tsweb/cypress/integration-tests/netflow_external_subnet.cy.tsweb/cypress/integration-tests/netflow_table.cy.tsweb/cypress/integration-tests/netflow_zone_multiCluster.cy.tsweb/cypress/integration-tests/quickFilters.cy.tsweb/cypress/integration-tests/table_queryopts.cy.tsweb/cypress/views/netflow-page.tsweb/cypress/views/netobserv.ts
Amoghrd
force-pushed
the
improvements-pf4
branch
2 times, most recently
from
dd76014 to
6830f8d
Compare
|
@Amoghrd - I am assuming you ran the tests on 4.14 cluster to verify the tests are working well. /lgtm |
|
In the process of doing that |
|
New changes are detected. LGTM label has been removed. |
|
Jenkins run: noo-frontend-tests#80 |
|
@Amoghrd: 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. |
2 participants
Description
Improve Cypress tests similar to #1514
PR does not include TLS automation since its not going to be cherry-picked to main-pf4 branch
Dependencies
n/a
Checklist
Summary by CodeRabbit
Release Notes
Tests
Chores