fix: Fix the S3 error case #39240
fix: Fix the S3 error case #39240
Conversation
WalkthroughThe changes refactor the Cypress test script for CRUD page generation by replacing direct Cypress commands with higher-level functions from the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Cypress Test
participant DS as dataSources Module
participant agHelper as agHelper
Test->>DS: GeneratePageForDS(datasourceName)
Test->>DS: TestDatasource(true)
DS->>agHelper: WaitUntilAllToastsDisappear()
Test->>DS: SaveDatasource()
DS->>agHelper: WaitUntilAllToastsDisappear()
Possibly related PRs
Suggested labels
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=50 update_snapshot=false specs_to_run=cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/13303545136. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js (2)
48-49: Good addition of toast wait, but consider using a more robust approach.While waiting for toasts to disappear is good, consider using Cypress's built-in wait mechanisms or data-testid attributes for more reliable test synchronization.
- agHelper.WaitUntilAllToastsDisappear(); + cy.waitUntil(() => !cy.get('[data-testid="t--toast-container"]').should('not.exist'), { + timeout: 5000, + interval: 500, + });
133-138: Good addition of toast waits, but consolidate the operations.The test and save operations with their respective toast waits could be combined into a single helper function for better maintainability.
- dataSources.TestDatasource(true); - agHelper.WaitUntilAllToastsDisappear(); - dataSources.SaveDatasource(); - agHelper.WaitUntilAllToastsDisappear(); + dataSources.TestSaveDatasource();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js(4 hunks)app/client/cypress/support/Pages/DataSources.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
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.
app/client/cypress/support/Pages/DataSources.tsapp/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js (2)
9-9: LGTM! Good addition of assertHelper.The import of assertHelper improves test reliability by providing better assertion handling capabilities.
85-85: LGTM! Good refactor to use helper function.Replacing direct navigation with
dataSources.GeneratePageForDSimproves code maintainability and reusability.
| public _testDs = ".t--test-datasource"; | ||
| public _saveDs = ".t--save-datasource"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider keeping selectors private and exposing through methods.
While making selectors public enables direct access, it breaks encapsulation. Consider keeping them private and exposing functionality through methods.
- public _testDs = ".t--test-datasource";
- public _saveDs = ".t--save-datasource";
+ private _testDs = ".t--test-datasource";
+ private _saveDs = ".t--save-datasource";
+ public getTestDatasourceSelector() { return this._testDs; }
+ public getSaveDatasourceSelector() { return this._saveDs; }📝 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.
| public _testDs = ".t--test-datasource"; | |
| public _saveDs = ".t--save-datasource"; | |
| private _testDs = ".t--test-datasource"; | |
| private _saveDs = ".t--save-datasource"; | |
| public getTestDatasourceSelector() { return this._testDs; } | |
| public getSaveDatasourceSelector() { return this._saveDs; } |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13303545136. |
Description
Fix: Changed to aghelper functions and added check for remove all toast message.
Fixes # https://app.zenhub.com/workspaces/qa-63316faf86bb2e170ed2e46b/issues/gh/appsmithorg/appsmith/39239
Automation
/ok-to-test tags="@tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13303544816
Commit: c4ff737
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Thu, 13 Feb 2025 09:21:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Tests
Refactor