fix: Fix copy issue in embedsetting spec#38590
Conversation
WalkthroughThe pull request modifies the Cypress end-to-end test specification for Embed Settings, introducing a new approach to handling clipboard data. The changes focus on retrieving clipboard content using the Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
/ci-test-limit-count run_count=1 update_snapshot=true specs_to_run=cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js |
![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)
app/client/cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js (1)
Line range hint
82-123: Restructure test cases for better maintainability.The test file combines multiple test scenarios into a single "it" block. Consider splitting these into separate test cases for better maintainability and clearer failure reporting:
- "Limit embedding"
- "Allow embedding everywhere"
- "Disable everywhere"
- it("1. Limit embedding then Allow embedding everywhere then Disable everywhere", function () { + it("1. should limit embedding to specific origins", function () { // First scenario code + }); + + it("2. should allow embedding everywhere", function () { // Second scenario code + }); + + it("3. should disable embedding everywhere", function () { // Third scenario code });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js (2)
21-22: LGTM! Variable declaration is appropriate.The
clipboardDatavariable is correctly scoped at the test suite level.
36-36: LGTM! Permission handling is appropriate.The Chrome copy permission is correctly set up before the test execution.
app/client/cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js
Show resolved
Hide resolved
|
|
||
| cy.window().then((win) => { | ||
| cy.stub(win.navigator.clipboard, "writeText").as("deployUrl").resolves(); | ||
| new Cypress.Promise((resolve, reject) => { | ||
| win.navigator.clipboard | ||
| .readText() | ||
| .then(resolve) | ||
| .catch(reject); | ||
| }).then((text) => { | ||
| clipboardData = text; // Store the clipboard content in a variable | ||
| cy.log(`Clipboard Content: ${clipboardData}`); // Log clipboard content | ||
| expect(clipboardData).to.equal("Expected clipboard text"); // Add assertions if needed | ||
| }); | ||
| }); | ||
|
|
||
| // Log clipboard data after it's been set | ||
| cy.then(() => { | ||
| cy.log(`Stored Clipboard Data: ${clipboardData}`); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor clipboard handling to follow best practices.
Several issues need to be addressed:
- Avoid hardcoded assertion strings
- Complex Promise handling could be simplified
- Remove unnecessary logging
- cy.window().then((win) => {
- new Cypress.Promise((resolve, reject) => {
- win.navigator.clipboard
- .readText()
- .then(resolve)
- .catch(reject);
- }).then((text) => {
- clipboardData = text;
- cy.log(`Clipboard Content: ${clipboardData}`);
- expect(clipboardData).to.equal("Expected clipboard text");
- });
- });
-
- cy.then(() => {
- cy.log(`Stored Clipboard Data: ${clipboardData}`);
- });
+ cy.window().then((win) => {
+ return win.navigator.clipboard.readText();
+ }).then((text) => {
+ clipboardData = text;
+ expect(clipboardData).to.be.a('string').and.not.be.empty;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cy.window().then((win) => { | |
| cy.stub(win.navigator.clipboard, "writeText").as("deployUrl").resolves(); | |
| new Cypress.Promise((resolve, reject) => { | |
| win.navigator.clipboard | |
| .readText() | |
| .then(resolve) | |
| .catch(reject); | |
| }).then((text) => { | |
| clipboardData = text; // Store the clipboard content in a variable | |
| cy.log(`Clipboard Content: ${clipboardData}`); // Log clipboard content | |
| expect(clipboardData).to.equal("Expected clipboard text"); // Add assertions if needed | |
| }); | |
| }); | |
| // Log clipboard data after it's been set | |
| cy.then(() => { | |
| cy.log(`Stored Clipboard Data: ${clipboardData}`); | |
| }); | |
| cy.window().then((win) => { | |
| return win.navigator.clipboard.readText(); | |
| }).then((text) => { | |
| clipboardData = text; | |
| expect(clipboardData).to.be.a('string').and.not.be.empty; | |
| }); |
|
/ci-test-limit runId=12739901097 |
9 similar comments
|
/ci-test-limit runId=12739901097 |
|
/ci-test-limit runId=12739901097 |
|
/ci-test-limit runId=12739901097 |
|
/ci-test-limit runId=12739901097 |
|
/ci-test-limit runId=12739901097 |
|
/ci-test-limit runId=12739901097 |
|
/ci-test-limit runId=12739901097 |
|
/ci-test-limit runId=12739901097 |
|
/ci-test-limit runId=12739901097 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/cypress/limited-tests.txt (1)
5-5: Consider adding a brief comment about the test's purpose.To improve maintainability, consider adding a brief comment above the test path explaining why this test is included in limited tests.
Add a comment like this:
+# Test for Chrome browser clipboard functionality in embed settings cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/limited-tests.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/limited-tests.txt (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
🔇 Additional comments (1)
app/client/cypress/limited-tests.txt (1)
5-5: LGTM! Test path aligns with PR objectives.The path correctly points to the EmbedSettings test that's being modified for clipboard functionality.
Let's verify the test file exists and is unique:
✅ Verification successful
Test file path verified and unique
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the test file exists and there are no duplicates fd "EmbedSettings_spec.js" | wc -lLength of output: 36
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/client/cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js (2)
54-69: 🛠️ Refactor suggestionSimplify clipboard handling and remove unnecessary logging
The clipboard handling implementation is overly complex and includes unnecessary logging.
- cy.window().then((win) => { - new Cypress.Promise((resolve, reject) => { - win.navigator.clipboard.readText().then(resolve).catch(reject); - }).then((text) => { - clipboardData = text; - cy.log(`Clipboard Content: ${clipboardData}`); - expect(clipboardData).to.equal("Expected clipboard text"); - }); - }); - - cy.then(() => { - cy.log(`Stored Clipboard Data: ${clipboardData}`); - }); + cy.window().then((win) => { + return win.navigator.clipboard.readText(); + }).then((text) => { + clipboardData = text; + expect(clipboardData).to.be.a('string').and.not.be.empty; + });
86-87: 🛠️ Refactor suggestionReplace explicit waits with proper Cypress assertions
Multiple instances of explicit waits (
cy.waitandSleep) should be replaced with proper Cypress assertions.- _.agHelper.Sleep(2000); - cy.visit(clipboardData, { timeout: 60000 }); + cy.visit(clipboardData); + cy.get('body', { timeout: Cypress.config('defaultCommandTimeout') }) + .should('be.visible');Also applies to: 101-101, 115-117
🧹 Nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js (1)
Line range hint
42-43: Use data-testid for element selectionReplace class-based selector with a data-testid attribute for better test maintainability.
- cy.get(".admin-settings-menu-option").click(); + cy.get("[data-testid='admin-settings-menu']").click();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (1)
app/client/cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js (1)
21-22: LGTM: Clipboard permission setupThe clipboard permission setup and variable declaration look good. The
GiveChromeCopyPermission()helper ensures proper clipboard access.Also applies to: 36-36
## Description RCA: Found the copy the clipboard is not working in chrome. Have enabled spec specific permission for copy and the store the cypress variable in local variable to get firm run results. Fixes # https://app.zenhub.com/workspaces/qa-63316faf86bb2e170ed2e46b/issues/gh/appsmithorg/appsmith/38589 ## Automation /ok-to-test tags="@tag.Settings" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12740837386> > Commit: e1903db > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12740837386&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Settings` > Spec: > <hr>Mon, 13 Jan 2025 05:11:28 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Updated clipboard handling in end-to-end testing - Improved clipboard permission and content retrieval for test scenarios <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
RCA: Found the copy the clipboard is not working in chrome. Have enabled spec specific permission for copy and the store the cypress variable in local variable to get firm run results.
Fixes # https://app.zenhub.com/workspaces/qa-63316faf86bb2e170ed2e46b/issues/gh/appsmithorg/appsmith/38589
Automation
/ok-to-test tags="@tag.Settings"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12740837386
Commit: e1903db
Cypress dashboard.
Tags:
@tag.SettingsSpec:
Mon, 13 Jan 2025 05:11:28 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit