Add a new function to match selector text #1708
Conversation
WalkthroughReplaced direct text-existence checks in cypress/e2e/models/migration/applicationinventory/analysis.ts with the new helper verifySelectorText, using the shared actionMenuItem selector and values from appInventoryKebab. Added verifySelectorText(expectedText: string, selector: string, shouldExist: boolean) to cypress/utils/utils.ts (queries elements by selector, trims textContent, and asserts exact-match presence/absence). Extended appInventoryKebab in cypress/e2e/types/constants.ts with manageCredentials = "Manage credentials" and delete = "Delete". No public/exported signatures were changed. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 (1)
cypress/e2e/models/migration/applicationinventory/analysis.ts (1)
27-27: LGTM with dependency on fixingmatchText.The refactor from
doesExistTexttomatchTextimproves precision by using anchored regex patterns for exact text matching. The imports and usage pattern are correct—passing menu item text and RBAC rule booleans to conditionally verify presence/absence of menu items.Note: The
matchTextfunction inutils.tshas a critical bug (unusedtoBePresentparameter) that must be fixed for these assertions to work correctly.Optional: Consider using constants for all menu items for consistency:
// In constants file, add: export const appInventoryKebab = { import: "Import", manageImports: "Manage application imports", manageCredentials: "Manage credentials", // Add this delete: "Delete", // Add this }; // Then use: matchText(appInventoryKebab.manageCredentials, rbacRules["Top action menu"]["Manage credentials"]); matchText(appInventoryKebab.delete, rbacRules["Top action menu"]["Delete"]);Also applies to: 40-40, 499-505
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/models/migration/applicationinventory/analysis.ts(3 hunks)cypress/utils/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cypress/e2e/models/migration/applicationinventory/analysis.ts (1)
cypress/utils/utils.ts (1)
matchText(1602-1604)
🪛 ast-grep (0.39.6)
cypress/utils/utils.ts
[warning] 1602-1602: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${str}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
Signed-off-by: Nandini Chandra <[email protected]>
nachandr
force-pushed
the
main_architect_test
branch
from
4ef6972 to
a9b15c7
Compare
nachandr
changed the title
nachandr
changed the title
|
ibragins
left a comment
ibragins
left a comment
There was a problem hiding this comment.
This function can be useful, but I prefer to use old approach in this particular case
| matchText(appInventoryKebab.import, rbacRules["Top action menu"]["Import"]); | ||
| matchText( | ||
| appInventoryKebab.manageImports, | ||
| rbacRules["Top action menu"]["Manage application imports"] | ||
| ); | ||
| doesExistText("Manage credentials", rbacRules["Top action menu"]["Manage credentials"]); | ||
| doesExistText("Delete", rbacRules["Top action menu"]["Delete"]); | ||
| matchText("Manage credentials", rbacRules["Top action menu"]["Manage credentials"]); | ||
| matchText("Delete", rbacRules["Top action menu"]["Delete"]); |
There was a problem hiding this comment.
In this particular case I would like to see exact match, not something matching regular expression. Even a typo can be missed in case of using REGEXP.
|
Hi @nachandr If we are going to keep both methods then let's rename it to something that others can find and use . |
Hi @sshveta , IIUC Nandini's method is matching not the whole text, but regular expression instead. So that is OLD method should be renamed |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/models/migration/applicationinventory/analysis.ts(4 hunks)cypress/utils/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/utils/utils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
cypress/e2e/models/migration/applicationinventory/analysis.ts (1)
cypress/e2e/views/common.view.ts (1)
actionMenuItem(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (1)
cypress/e2e/models/migration/applicationinventory/analysis.ts (1)
27-27: LGTM! Import changes align with exact text matching approach.The addition of
matchText,appInventoryKebab, andactionMenuItemimports supports the transition from substring to exact text matching, which addresses the issue where label changes (e.g., "Import" → "Import applications from CSV") broke existing tests.Also applies to: 40-40, 90-90
nachandr
force-pushed
the
main_architect_test
branch
from
ce93709 to
f7f37dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cypress/e2e/models/migration/applicationinventory/analysis.ts (1)
499-514: Address hardcoded "Delete" string and enum definition gap.Line 514 uses a hardcoded "Delete" string instead of a constant from the
appInventoryKebabenum. The enum incypress/e2e/types/constants.ts(lines 248-252) currently lacks adeleteproperty and must be extended.Required changes:
- Add to
appInventoryKebabenum incypress/e2e/types/constants.ts:export enum appInventoryKebab { manageImports = "Manage application imports", import = "Import applications from CSV", managecredentials = "Manage credentials", + delete = "Delete", }
- Update line 514 in
cypress/e2e/models/migration/applicationinventory/analysis.ts:- verifySelectorText("Delete", actionMenuItem, rbacRules["Top action menu"]["Delete"]); + verifySelectorText(kebab.delete, actionMenuItem, rbacRules["Top action menu"]["Delete"]);
- Optional: Add JSDoc to
verifySelectorTextincypress/utils/utils.ts(line 1603) to document the exact-match behavior and clarify when to use this versus regex-based alternatives.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cypress/e2e/models/migration/applicationinventory/analysis.ts(3 hunks)cypress/e2e/types/constants.ts(1 hunks)cypress/utils/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/utils/utils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
cypress/e2e/models/migration/applicationinventory/analysis.ts (2)
cypress/utils/utils.ts (1)
verifySelectorText(1603-1619)cypress/e2e/views/common.view.ts (1)
actionMenuItem(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (1)
cypress/e2e/models/migration/applicationinventory/analysis.ts (1)
34-34: LGTM on the new imports.The imports for
verifySelectorText, thekebabalias forappInventoryKebab, andactionMenuItemare correctly added and appropriately used in the updated validation logic.Also applies to: 40-40, 90-90
nachandr
force-pushed
the
main_architect_test
branch
from
f7f37dc to
73f0442
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cypress/utils/utils.ts (2)
1602-1619: Consider a more descriptive function name that indicates exact text matching.Based on the PR discussion, reviewers suggested a clearer, more discoverable name such as
doesExactTextExist()orverifyExactSelectorText()to distinguish this function from other text-matching utilities likedoesExistText()which uses regex matching. The current name doesn't clearly convey that it performs exact text matching rather than substring or pattern matching.Apply this diff to use a clearer name:
-export function verifySelectorText( +export function verifyExactSelectorText( expectedText: string, selector: string, toBePresent: boolean ): void { cy.get(selector).then(($elements) => { const texts = Array.from($elements).map((el) => el.textContent?.trim() ?? ""); const matched = texts.includes(expectedText); if (toBePresent) { expect(matched, `Expected one of the elements to have exact text "${expectedText}"`).to .be.true; } else { expect(matched, `Expected no element to have exact text "${expectedText}"`).to.be.false; } }); }Don't forget to update the import and usage in
cypress/e2e/models/migration/applicationinventory/analysis.tsto match the new name.Based on learnings
1602-1619: Add documentation explaining when to use this function versus other text-matching utilities.As noted in the PR discussion, developers need guidance on when to use this exact text matcher versus other similar functions like
doesExistText()(regex-based) orvalidateTextPresence()(substring-based). Adding a JSDoc comment would improve discoverability and prevent confusion.Apply this diff to add documentation:
+/** + * Verifies that at least one element matched by the selector contains the exact expected text. + * Unlike doesExistText() which uses regex matching or validateTextPresence() which checks substrings, + * this function performs exact text matching after trimming whitespace. + * + * Use this when you need to verify exact menu labels, button text, or other UI strings that must match precisely. + * + * @param expectedText - The exact text to match (after trimming) + * @param selector - CSS selector to find elements + * @param toBePresent - true to assert presence, false to assert absence + */ export function verifySelectorText( expectedText: string, selector: string, toBePresent: boolean ): void {Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cypress/e2e/models/migration/applicationinventory/analysis.ts(3 hunks)cypress/e2e/types/constants.ts(1 hunks)cypress/utils/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/e2e/models/migration/applicationinventory/analysis.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (1)
cypress/e2e/types/constants.ts (1)
251-252: LGTM!The enum additions follow proper naming conventions with camelCase (
manageCredentials) consistent with the existingmanageImportsmember, and both requested members (manageCredentialsanddelete) have been added correctly.
cypress/utils/utils.ts
Outdated
| // Function to verify if selector has exact text | ||
| export function verifySelectorText( | ||
| expectedText: string, | ||
| selector: string, | ||
| toBePresent: boolean | ||
| ): void { | ||
| cy.get(selector).then(($elements) => { | ||
| const texts = Array.from($elements).map((el) => el.textContent?.trim() ?? ""); | ||
| const matched = texts.includes(expectedText); | ||
|
|
||
| if (toBePresent) { | ||
| expect(matched, `Expected one of the elements to have exact text "${expectedText}"`).to | ||
| .be.true; | ||
| } else { | ||
| expect(matched, `Expected no element to have exact text "${expectedText}"`).to.be.false; | ||
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
cy.get(selector).should(toBePresent ? "have.text" : "not.have.text", expectedText);
This is how I'd do this if you want to use selector
There was a problem hiding this comment.
This is how it can work without selector, regexp is indeed required here
export function doesExistText(str: string, toBePresent: boolean): void {
const regex = new RegExp(`^${str}$`);
cy.contains(regex).should(toBePresent ? "exist" : "not.exist");
}
There was a problem hiding this comment.
cy.get(selector).should(toBePresent ? "have.text" : "not.have.text", expectedText);This is how I'd do this if you want to use selector
Thanks @ibragins .
- The option without the regex also works well.
- With this option, `cy.get(selector).should("have.text") checks the concatenated text of all matched elements, not per element.
There was a problem hiding this comment.
@nachandr definitely element by particular selector should be unique
nachandr
force-pushed
the
main_architect_test
branch
2 times, most recently
from
98cf1a4 to
9c25f3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cypress/utils/utils.ts (1)
1608-1618: Consider using Cypress's retry-able assertions for better test stability.The current implementation uses
cy.get().then()with imperativeexpect(), which won't retry if elements are still loading or text is being updated. This can cause flaky tests in dynamic UIs.Apply this diff to leverage Cypress's built-in retry-ability:
export function verifySelectorText( expectedText: string, selector: string, toBePresent: boolean ): void { - cy.get(selector).then(($elements) => { - const texts = Array.from($elements).map((el) => el.textContent?.trim() ?? ""); - const matched = texts.includes(expectedText); - - if (toBePresent) { - expect(matched, `Expected one of the elements to have exact text "${expectedText}"`).to - .be.true; - } else { - expect(matched, `Expected no element to have exact text "${expectedText}"`).to.be.false; - } - }); + cy.get(selector).should(($elements) => { + const texts = Array.from($elements).map((el) => el.textContent?.trim() ?? ""); + const matched = texts.includes(expectedText); + + if (toBePresent) { + expect(matched, `Expected one of the elements to have exact text "${expectedText}"`).to + .be.true; + } else { + expect(matched, `Expected no element to have exact text "${expectedText}"`).to.be.false; + } + }); }This change enables automatic retrying until the assertion passes or times out, improving reliability when dealing with asynchronously rendered content.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cypress/e2e/models/migration/applicationinventory/analysis.ts(3 hunks)cypress/e2e/types/constants.ts(1 hunks)cypress/utils/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/e2e/models/migration/applicationinventory/analysis.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (1)
cypress/e2e/types/constants.ts (1)
251-252: LGTM! Naming consistency issue resolved.The enum additions follow proper camelCase naming convention (consistent with
manageImportson line 249), and adding these constants eliminates hardcoded strings in the codebase.
nachandr
force-pushed
the
main_architect_test
branch
from
9c25f3a to
6c2a5ff
Compare
|
nachandr
force-pushed
the
main_architect_test
branch
from
6c2a5ff to
c442029
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cypress/e2e/models/migration/applicationinventory/analysis.ts(3 hunks)cypress/e2e/types/constants.ts(1 hunks)cypress/utils/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/e2e/models/migration/applicationinventory/analysis.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (2)
cypress/utils/utils.ts (1)
1602-1610: LGTM! Well-documented function.The JSDoc documentation clearly explains the function's purpose and distinguishes it from the existing
doesExistTexthelper. This addresses the previous review feedback about adding documentation.cypress/e2e/types/constants.ts (1)
251-252: LGTM! Consistent naming convention.The new enum members follow the existing naming pattern: camelCase for multi-word actions (
manageCredentials) and lowercase for single-word actions (delete). The string values correctly represent the menu labels from the App Inventory kebab menu.
Signed-off-by: Nandini Chandra <[email protected]>
nachandr
force-pushed
the
main_architect_test
branch
from
c442029 to
f22ebc9
Compare
nachandr
changed the title
3 participants
In 8.0.1, the 'Import' top kebab menu option on the App Inventory page was replaced by 'Import applications from CSV'. Currently, only a sub text is matched , but the not the entire text. So, I've added a new function to match the exact text and updated the test to use the new function.
Summary by CodeRabbit