test: Extract pageId with regex to cover more cases
#34521
Conversation
WalkthroughThe changes primarily focus on abstracting URL page ID extraction methods for consistency and maintainability across multiple files. Helper functions are introduced and updated to manage these extractions, supporting various URL formats. Additionally, the updates involve refactoring and renaming variables for better readability and maintaining deprecated path structures. Changes
Sequence DiagramsSilently ignored as diagrams do not fit the context of these changes. Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
skip-changelog
sharat87
added
ok-to-test
sharat87
added
ok-to-test
sharat87
added
ok-to-test
sharat87
added
ok-to-test
sharat87
added
ok-to-test
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (6)
app/client/cypress/support/Pages/AggregateHelper.ts (6)
Line range hint
866-869: Refactor: Omit unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - options = {}; - } - } + } + options = {};
Line range hint
894-894: Refactor: Move default parameter to the end.The default parameter should follow the last required parameter or should be a required parameter.
- waitAfterClick = true, waitTimeInterval = 500, type = "click"; + waitAfterClick = true;
Line range hint
1521-1522: Refactor: Move default parameter to the end.The default parameter should follow the last required parameter or should be a required parameter.
- hierarchy = [], propFieldName, toggleEle = null, valueToValidate, widgetName, widgetType = EntityType.Widget, + hierarchy = [];
Line range hint
1551-1552: Refactor: Simplify boolean literals in conditional expression.Simplify your code by directly assigning the result without using a ternary operator.
- return this.GetElement(selector) - .eq(index) - .should(disabled ? "have.attr" : "not.have.attr", "disabled"); + const assertion = disabled ? "have.attr" : "not.have.attr"; + return this.GetElement(selector).eq(index).should(assertion, "disabled");
Line range hint
1589-1593: Refactor: Omit unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - this.GetElement(selector) - .uncheck({ force: true }) - .should("not.be.checked"); - } + } + this.GetElement(selector).uncheck({ force: true }).should("not.be.checked");
Line range hint
1605-1610: Refactor: Omit unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - $element[0].scrollIntoView(); - return $element; - } - }) - .click({ force: force }) - .wait(waitTimeInterval); + } + $element[0].scrollIntoView(); + return $element; +}) + .click({ force: force }) + .wait(waitTimeInterval);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationURL_spec.js (3 hunks)
- app/client/cypress/support/Pages/AggregateHelper.ts (1 hunks)
- app/client/cypress/support/Pages/AppSettings/AppSettings.ts (1 hunks)
- app/client/src/ce/constants/routes/appRoutes.ts (1 hunks)
- app/client/src/ce/pages/Editor/Explorer/helpers.test.ts (3 hunks)
- app/client/src/navigation/FocusEntity.test.ts (19 hunks)
- app/client/src/pages/tests/slug.test.tsx (2 hunks)
Additional context used
Biome
app/client/cypress/support/Pages/AggregateHelper.ts
[error] 866-869: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 894-894: This default parameter should follow the last required parameter or should be a required parameter.
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.(lint/style/useDefaultParameterLast)
[error] 1521-1522: This default parameter should follow the last required parameter or should be a required parameter.
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.(lint/style/useDefaultParameterLast)
[error] 1551-1552: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
[error] 1589-1593: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 1605-1610: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
Additional comments not posted (14)
app/client/src/ce/pages/Editor/Explorer/helpers.test.ts (2)
6-7: LGTM!The
applicationIdandpageIdconstants are appropriately added for use in the test cases.
45-45: LGTM!The usage of
applicationIdandpageIdconstants in the test cases is correct and consistent.Also applies to: 55-55
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationURL_spec.js (4)
19-19: LGTM!The usage of
agHelper.extractPageIdFromUrlto extract thepageIdfrom the URL is correct and consistent with the purpose of the test case.
25-25: LGTM!The usage of
agHelper.extractPageIdFromUrlto extract thepageIdfrom the URL is correct and consistent with the purpose of the test case.
41-41: LGTM!The usage of
agHelper.extractPageIdFromUrlto extract thepageIdfrom the URL is correct and consistent with the purpose of the test case.
56-56: LGTM!The usage of
agHelper.extractPageIdFromUrlto extract thepageIdfrom the URL is correct and consistent with the purpose of the test case.app/client/cypress/support/Pages/AppSettings/AppSettings.ts (1)
126-126: LGTM!The usage of
agHelper.extractPageIdFromUrlto extract thepageIdfrom the URL is correct and consistent with the purpose of theCheckUrlmethod.app/client/src/pages/tests/slug.test.tsx (2)
80-81: LGTM!The
applicationIdandpageIdconstants are appropriately added for use in the test cases.
117-119: LGTM!The usage of
applicationIdandpageIdconstants in the test cases is correct and consistent.app/client/src/ce/constants/routes/appRoutes.ts (2)
31-31: EnsureID_EXTRACTION_REGEXis correctly defined.The addition of
ID_EXTRACTION_REGEXtoBUILDER_PATH_DEPRECATEDensures consistent ID extraction. Verify thatID_EXTRACTION_REGEXcorrectly handles Mongo ObjectIds and UUIDs.
32-32: EnsureID_EXTRACTION_REGEXis correctly defined.The addition of
ID_EXTRACTION_REGEXtoVIEWER_PATH_DEPRECATEDensures consistent ID extraction. Verify thatID_EXTRACTION_REGEXcorrectly handles Mongo ObjectIds and UUIDs.app/client/src/navigation/FocusEntity.test.ts (1)
10-11: Good practice: Use of constants for IDs.The use of
applicationIdandpageIdconstants improves maintainability and readability by avoiding hardcoded values.app/client/cypress/support/Pages/AggregateHelper.ts (2)
136-141: Documentation: Ensure the comment accurately describes the method.The comment provides a clear description of the
extractPageIdFromUrlmethod, including the supported URL formats. Ensure it remains accurate as the method evolves.
142-146: Regex-based extraction for robustness.The
extractPageIdFromUrlmethod uses a regex to handle various URL formats, improving the robustness of the ID extraction process. Ensure the regex covers all required cases.
) There's a few places in Cypress tests that are trying to extract the page ID using `.split`, especially with just the path information, instead of the whole URL. So this PR changes the extraction implementation to use a regex, to support all three cases we need: 1. Full absolute application+page URL. 2. Just the path of an application+page URL. 3. Just the path of an application with a custom slug. 4. Full absolute application with a custom slug. (Supported, but we don't need this today). We've also fixed the URL parsing for the (very) old application URLs that didn't use slugs in the URL at all. This was making `FocusEntity.test.ts` fail. Fixed that test as well as improved it's error reporting. Before this PR:  After this PR:  No conflicts to EE. /test all <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9713573983> > Commit: 49edbce > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9713573983&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `` <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Improved URL handling by centralizing page ID extraction logic across various tests and components. - Updated deprecated path constants to include ID extraction patterns for better consistency. - Enhanced code readability and maintainability by moving page ID extraction to a helper method. - **Tests** - Modified test cases to dynamically set `applicationId` and `pageId` instead of hardcoding values, ensuring more flexible and maintainable test scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
There's a few places in Cypress tests that are trying to extract the page ID using
.split, especially with just the path information, instead of the whole URL. So this PR changes the extraction implementation to use a regex, to support all three cases we need:We've also fixed the URL parsing for the (very) old application URLs that didn't use slugs in the URL at all. This was making
FocusEntity.test.tsfail.Fixed that test as well as improved it's error reporting.
Before this PR:
After this PR:
No conflicts to EE.
/test all
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9713573983
Commit: 49edbce
Cypress dashboard.
Tags: ``
Summary by CodeRabbit
Refactor
Tests
applicationIdandpageIdinstead of hardcoding values, ensuring more flexible and maintainable test scenarios.