-
Notifications
You must be signed in to change notification settings - Fork 665
CONSOLE-4912: update machine config tests #15759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@yapei: This pull request references CONSOLE-4912 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.21.0" version, but no target version was set. In 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. |
WalkthroughAdded guided-tour cleanup and an afterEach error check to a Cypress machine-config test; introduced a helper to validate machine config file details and extended a test to fetch and verify machine config data via an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/packages/integration-tests-cypress/tests/app/machine-config.cy.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/integration-tests-cypress/tests/app/machine-config.cy.ts
🔇 Additional comments (2)
frontend/packages/integration-tests-cypress/tests/app/machine-config.cy.ts (2)
3-3: LGTM: Guided tour cleanup import added.The import is correctly used in the test setup to ensure guided tours don't interfere with test execution.
31-31: LGTM: Guided tour cleanup in test setup.Properly dismissing the guided tour before test execution prevents potential interference with UI interactions.
frontend/packages/integration-tests-cypress/tests/app/machine-config.cy.ts
Show resolved
Hide resolved
frontend/packages/integration-tests-cypress/tests/app/machine-config.cy.ts
Show resolved
Hide resolved
|
@rhamilto could you help give a review? Thanks! |
| const MC_C2C = '.co-copy-to-clipboard__text'; | ||
| const checkMachineConfigDetails = (mode, overwrite, content) => { | ||
| cy.byTestID(MC_CONFIG_FILE_PATH_ID).scrollIntoView(); | ||
| cy.get('button[aria-label="public~Info"]').first().click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the i18n namespace in the aria-label is a bug and should be fixed. it should be aria-label={t('public~Info')}. If that is not specific enough we can add a test id for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! wasn't aware that we have a bug here, will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/integration-tests-cypress/tests/app/machine-config.cy.ts (1)
11-26: Scope assertions to the config-file section to avoid accidental matches
checkMachineConfigDetailsdoes the right logical checks, but the selectors are fairly broad:
cy.contains(mode)/cy.contains(overwrite.toString())search the entire page, so another element showing the same text could make the test pass even if the popover content is wrong.cy.get('code').first()will grab the first<code>element anywhere, not necessarily the one associated withMC_CONFIG_FILE_PATH_ID.Consider scoping these assertions to the configuration-file row (for example by using
cy.byTestID(MC_CONFIG_FILE_PATH_ID).parent().within(...)or a similar container) so the mode/overwrite and code checks are guaranteed to apply to the targeted file entry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
frontend/packages/integration-tests-cypress/tests/app/machine-config.cy.ts(2 hunks)frontend/public/components/machine-config.tsx(1 hunks)frontend/public/locales/en/public.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/integration-tests-cypress/tests/app/machine-config.cy.tsfrontend/public/components/machine-config.tsxfrontend/public/locales/en/public.json
🧬 Code graph analysis (1)
frontend/public/components/machine-config.tsx (1)
frontend/__mocks__/i18next.ts (1)
t(8-14)
🔇 Additional comments (4)
frontend/public/components/machine-config.tsx (1)
80-107: Localized ARIA label correctly wired to i18nSwitching the Info button
aria-labeltot('public~Info')aligns with existing i18n usage and keeps the control accessible without changing behavior. Looks good.frontend/public/locales/en/public.json (1)
724-736: Info translation entry is consistent with usageAdding
"Info": "Info"under thepublicnamespace correctly supportst('public~Info')in the UI; no issues seen.frontend/packages/integration-tests-cypress/tests/app/machine-config.cy.ts (2)
3-37: Guided tour cleanup and global error check improve test stabilityClosing the guided tour in
beforeand runningcheckErrors()inafterEachshould reduce flakiness from overlays and surface console issues early. This setup looks solid.
46-59: Server-side file data verification is wired correctlyUsing
oc get mc ... -o jsonpath='{.spec.config.storage.files[0]}', parsing the result, asserting presence ofcontents,mode, andoverwrite, then delegating tocheckMachineConfigDetailsgives a good end-to-end check that the UI reflects the MachineConfig’s underlying data. This looks good and fits the PR’s goal of checking more details.
|
@logonoff comments resolved ~ PTAL, thanks! |
logonoff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, yapei The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
flaky test /retest-required |
|
/retest-required |
|
/verified by CI |
|
@yapei: This PR has been marked as verified by In 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. |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/label acknowledge-critical-fixes-only |
|
@yapei: The label(s) In 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 kubernetes-sigs/prow repository. |
|
/label px-approved |
|
@yapei: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |

machine-config.cy.tsto check exact content and more details