NETOBSERV-2653: TLS Tracking automation#1514
Open
Amoghrd wants to merge 3 commits into
Open
Conversation
Collaborator
|
@Amoghrd: This pull request references NETOBSERV-2653 which is a valid jira issue. 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. |
Contributor
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
[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 |
Amoghrd
changed the title
Amoghrd
force-pushed
the
TLS-UI
branch
2 times, most recently
from
3d32570 to
ba80cae
Compare
Amoghrd
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
web/cypress/integration-tests/tls_dashboards.cy.ts (1)
22-33: 💤 Low valueConsider adding explicit wait after scroll.
Line 30 scrolls to bottom, then line 32 immediately checks panels. If panels need time to render after scroll, add a wait:
cy.get('#content-scrollable').scrollTo('bottom') cy.wait(1000) // Allow panels to render cy.checkDashboards(TLSPanels)However, if
cy.checkDashboards()already includes retry logic, this may be unnecessary.🤖 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/tls_dashboards.cy.ts` around lines 22 - 33, After scrolling to bottom in the test (cy.get('#content-scrollable').scrollTo('bottom')), add a short explicit wait before calling cy.checkDashboards(TLSPanels) to allow panels to render (e.g., a cy.wait(1000)), or alternatively ensure the helper cy.checkDashboards (used earlier) includes retry/assertion logic to tolerate post-scroll rendering latency; update the test to either insert the wait after .scrollTo or enhance checkDashboards to retry until panels in TLSPanels are present.web/cypress/views/netobserv.ts (1)
61-61: 💤 Low valueConsider explicit undefined handling for environment variables.
The template literal
${Cypress.env('...')}convertsundefinedto"undefined"string. While safe, explicit checks would be clearer:if (Cypress.env('NOO_CATALOG_SOURCE') === 'upstream') {Also applies to: 73-73, 90-90
🤖 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/netobserv.ts` at line 61, The current checks use template literals like `${Cypress.env('NOO_CATALOG_SOURCE')}` which turn undefined into the string "undefined"; update the conditionals to compare the raw env value directly (e.g., Cypress.env('NOO_CATALOG_SOURCE') === 'upstream') and, where appropriate, add an explicit undefined/null guard (e.g., store const src = Cypress.env('NOO_CATALOG_SOURCE') and check src !== undefined && src === 'upstream') to avoid accidental stringified "undefined"; apply the same change for the other occurrences that use `${Cypress.env(...)}` (the checks around the same logic at the other two locations).
🤖 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/integration-tests/tls_tracking.cy.ts`:
- Around line 56-87: The test applies filters but never asserts any rows exist,
so the subsequent .each() checks can be skipped silently; update the TLS column
checks (the cy.get calls using '[data-test-td-column-id="TLSVersion"]',
'[data-test-td-column-id="TLSTypes"]', '[data-test-td-column-id="TLSGroup"]',
and '[data-test-td-column-id="TLSCipherSuite"]') to first assert there is at
least one cell (e.g., should('have.length.greaterThan', 0)) before calling
.each(), then keep the existing content/non-empty assertions inside the .each()
callbacks to ensure the filters returned results and the TLS data is validated.
In `@web/cypress/views/netobserv.ts`:
- Around line 96-101: Replace the unnecessary use of the any type in the
cy.adminCLI promise callback by changing the callback signature to use
Cypress.Exec (i.e., update the .then((result: any) => { ... }) handler to
.then((result: Cypress.Exec) => { ... }) so the CLI result is properly typed;
update the callback in the cy.adminCLI invocation inside netobserv.ts near the
block checking result.stdout/includes('netobserv-operator') and
result.stdout/includes('Succeeded') to use the Cypress.Exec type.
---
Nitpick comments:
In `@web/cypress/integration-tests/tls_dashboards.cy.ts`:
- Around line 22-33: After scrolling to bottom in the test
(cy.get('#content-scrollable').scrollTo('bottom')), add a short explicit wait
before calling cy.checkDashboards(TLSPanels) to allow panels to render (e.g., a
cy.wait(1000)), or alternatively ensure the helper cy.checkDashboards (used
earlier) includes retry/assertion logic to tolerate post-scroll rendering
latency; update the test to either insert the wait after .scrollTo or enhance
checkDashboards to retry until panels in TLSPanels are present.
In `@web/cypress/views/netobserv.ts`:
- Line 61: The current checks use template literals like
`${Cypress.env('NOO_CATALOG_SOURCE')}` which turn undefined into the string
"undefined"; update the conditionals to compare the raw env value directly
(e.g., Cypress.env('NOO_CATALOG_SOURCE') === 'upstream') and, where appropriate,
add an explicit undefined/null guard (e.g., store const src =
Cypress.env('NOO_CATALOG_SOURCE') and check src !== undefined && src ===
'upstream') to avoid accidental stringified "undefined"; apply the same change
for the other occurrences that use `${Cypress.env(...)}` (the checks around the
same logic at the other two locations).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dea91e58-4b0c-4800-b1ac-31b538760753
📒 Files selected for processing (6)
web/cypress/fixtures/flowcollector/fc_TLSTracking.yamlweb/cypress/integration-tests/dns_dashboards.cy.tsweb/cypress/integration-tests/tls_dashboards.cy.tsweb/cypress/integration-tests/tls_tracking.cy.tsweb/cypress/views/netflow-page.tsweb/cypress/views/netobserv.ts
Amoghrd
force-pushed
the
TLS-UI
branch
2 times, most recently
from
4975c1a to
533098e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main-pf5 #1514 +/- ##
============================================
- Coverage 52.53% 52.33% -0.21%
============================================
Files 232 232
Lines 12355 12454 +99
Branches 1551 1562 +11
============================================
+ Hits 6491 6518 +27
- Misses 5269 5337 +68
- Partials 595 599 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/cypress/integration-tests/tls_tracking.cy.ts (1)
81-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard remaining TLS column
.each()checks with non-empty assertions.Only
TLSVersionasserts non-empty before.each().TLSTypes,TLSGroup, andTLSCipherSuitecan still pass silently with zero matches.Suggested fix
- cy.get('[data-test-td-column-id="TLSTypes"]').each((td) => { + cy.get('[data-test-td-column-id="TLSTypes"]') + .should('have.length.greaterThan', 0) + .each((td) => { expect(td).to.contain('ServerHello') }) - cy.get('[data-test-td-column-id="TLSGroup"]').each((td) => { + cy.get('[data-test-td-column-id="TLSGroup"]') + .should('have.length.greaterThan', 0) + .each((td) => { expect(td.text().trim()).to.not.be.empty }) - cy.get('[data-test-td-column-id="TLSCipherSuite"]').each((td) => { + cy.get('[data-test-td-column-id="TLSCipherSuite"]') + .should('have.length.greaterThan', 0) + .each((td) => { expect(td.text().trim()).to.not.be.empty })As per coding guidelines, "web/cypress/**/*.ts: Verify E2E test stability, proper waits, and selector resilience".
🤖 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/tls_tracking.cy.ts` around lines 81 - 89, The three Cypress checks using the selectors TLSTypes, TLSGroup, and TLSCipherSuite can silently pass if there are zero matches; before calling .each() on cy.get('[data-test-td-column-id="TLSTypes"]'), cy.get('[data-test-td-column-id="TLSGroup"]'), and cy.get('[data-test-td-column-id="TLSCipherSuite"]') add a guard assertion that the query returns at least one element (e.g., .should('have.length.greaterThan', 0) or equivalent) so the subsequent .each() actually iterates over results, mirroring the existing TLSVersion non-empty check.
🧹 Nitpick comments (1)
web/cypress/integration-tests/netflow_table.cy.ts (1)
71-71: ⚡ Quick winInconsistent selector usage.
Line 71 uses a hard-coded selector
'#SrcK8S_Namespace[type="checkbox"]'while the rest of the test usescolSelectors. Line 81 references the same column viacolSelectors.srcNS. Use the centralized selector here for consistency.♻️ Proposed fix
- cy.get('#SrcK8S_Namespace[type="checkbox"]').uncheck() + cy.get(colSelectors.srcNS).uncheck()🤖 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/netflow_table.cy.ts` at line 71, Replace the hard-coded selector in the test (the cy.get('#SrcK8S_Namespace[type="checkbox"]').uncheck() call) with the centralized column selector used elsewhere; use colSelectors.srcNS (or the appropriate property on colSelectors referencing SrcK8S_Namespace) so the line becomes cy.get(colSelectors.srcNS).uncheck() to keep selector usage consistent with the rest of the test.
🤖 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/integration-tests/tls_tracking.cy.ts`:
- Around line 56-73: After typing the filters into
cy.get(filterSelectors.filterInput) (the two lines that call
.type("tls_version=...") and .type("tls_types=...")), add an explicit wait for
the Loki query response before opening the columns modal; intercept or alias the
Loki/network request used by the app (e.g., cy.intercept(...).as('lokiQuery'))
and call cy.wait('@lokiQuery') right after the second .type() to ensure the
filtered table has settled, then proceed to cy.openColumnsModal() and the
subsequent assertions on colSelectors.tlsCipherSuite, colSelectors.tlsGroup and
colSelectors.tlsTypes.
---
Duplicate comments:
In `@web/cypress/integration-tests/tls_tracking.cy.ts`:
- Around line 81-89: The three Cypress checks using the selectors TLSTypes,
TLSGroup, and TLSCipherSuite can silently pass if there are zero matches; before
calling .each() on cy.get('[data-test-td-column-id="TLSTypes"]'),
cy.get('[data-test-td-column-id="TLSGroup"]'), and
cy.get('[data-test-td-column-id="TLSCipherSuite"]') add a guard assertion that
the query returns at least one element (e.g., .should('have.length.greaterThan',
0) or equivalent) so the subsequent .each() actually iterates over results,
mirroring the existing TLSVersion non-empty check.
---
Nitpick comments:
In `@web/cypress/integration-tests/netflow_table.cy.ts`:
- Line 71: Replace the hard-coded selector in the test (the
cy.get('#SrcK8S_Namespace[type="checkbox"]').uncheck() call) with the
centralized column selector used elsewhere; use colSelectors.srcNS (or the
appropriate property on colSelectors referencing SrcK8S_Namespace) so the line
becomes cy.get(colSelectors.srcNS).uncheck() to keep selector usage consistent
with the rest of the test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f08411d-7a04-44c6-8f26-43303cd3f4bb
📒 Files selected for processing (12)
web/cypress/fixtures/flowcollector/fc_TLSTracking.yamlweb/cypress/integration-tests/dns_dashboards.cy.tsweb/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/table_queryopts.cy.tsweb/cypress/integration-tests/tls_dashboards.cy.tsweb/cypress/integration-tests/tls_tracking.cy.tsweb/cypress/views/netflow-page.tsweb/cypress/views/netobserv.ts
✅ Files skipped from review due to trivial changes (2)
- web/cypress/integration-tests/netflow_external_subnet.cy.ts
- web/cypress/integration-tests/dns_dashboards.cy.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- web/cypress/fixtures/flowcollector/fc_TLSTracking.yaml
- web/cypress/views/netflow-page.ts
- web/cypress/views/netobserv.ts
Member
Author
|
Trying to simplify install and visitFlowCollector funcs. Running the whole suite to check if it works |
Amoghrd
changed the title
Amoghrd
changed the title
Amoghrd
added
needs-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
data-test=thDependencies
#1478
Checklist
Test Results
Jenkins run: noo-frontend-tests#75/ Failing tests fixed and they are passing on rerun.
Also tested tests with downstream and upstream operator not installed prior to running tests. Both passed
Summary by CodeRabbit
Release Notes
New Features
Tests