test: added test for incompatible file #37323
Conversation
NandanAnantharamu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThis pull request introduces a new Cypress end-to-end test suite for validating the partial import functionality of an application. It includes a new test case that checks the handling of an incompatible JSON file during the import process. Additionally, a new JSON file is added to represent the application's structure, encompassing metadata, configuration, data sources, and page layouts. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
skip-changelog
|
/ci-test-limit-count run_count=25 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11783494846. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
app/client/cypress/fixtures/PartialImportAppNegative.json (1)
512-516: Remove unused variables in JSObject2The object contains unused variables myVar1 and myVar2.
Consider removing them if they're not needed for the test scenario.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts(1 hunks)app/client/cypress/fixtures/PartialImportAppNegative.json(1 hunks)app/client/cypress/limited-tests.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/cypress/limited-tests.txt
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts (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.
app/client/cypress/fixtures/PartialImportAppNegative.json (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.
🔇 Additional comments (4)
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts (3)
1-6: LGTM! Clean imports from ObjectsCore
The imports are well-organized and follow best practices by using the centralized ObjectsCore.
7-10: LGTM! Well-structured test suite declaration
The test suite has appropriate tags and a clear description.
11-13: LGTM! Proper test setup using helper method
The beforeEach hook appropriately uses the helper method for opening the import modal.
app/client/cypress/fixtures/PartialImportAppNegative.json (1)
2-4: Version mismatch in schema versions
The client schema version (1.0) and server schema version (11.0) have a significant gap, which could be intentional for testing incompatibility scenarios.
| it("1. Verify Importing App into Page shows error message for incompatible json file ", () => { | ||
| homePage.ImportApp("PartialImportAppNegative.json", "", true); | ||
| agHelper.ValidateToastMessage("Unable to import artifact in workspace The file is not compatible with the current partial import operation. Please check the file and try again.. {1}") | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test robustness with constants and additional assertions
While the test follows most best practices, consider these improvements:
- Move the error message to a constants file to avoid hardcoded strings in assertions
- Add assertions to verify the modal state after the error
Here's how you could improve it:
+ import { importExportConstants } from "../../../../support/Constants";
// ...
it("1. Verify Importing App into Page shows error message for incompatible json file ", () => {
homePage.ImportApp("PartialImportAppNegative.json", "", true);
- agHelper.ValidateToastMessage("Unable to import artifact in workspace The file is not compatible with the current partial import operation. Please check the file and try again.. {1}")
+ agHelper.ValidateToastMessage(importExportConstants.INCOMPATIBLE_JSON_FILE_ERROR);
+ // Add additional assertions
+ agHelper.AssertElementVisibility(partialImportExport.importModal);
+ agHelper.AssertElementVisibility(partialImportExport.errorBlock);
});Committable suggestion skipped: line range outside the PR's diff.
| "unpublishedCustomJSLibs": [ | ||
| { | ||
| "uidString": "jspdf_https://cdnjs.cloudflare.com/ajax/libs/jspdf/2.5.1/jspdf.umd.min.js" | ||
| } | ||
| ], | ||
| "publishedCustomJSLibs": [], |
There was a problem hiding this comment.
Security concern with external JS library
The unpublished custom JS library references an external CDN URL. Consider:
- Pinning to a specific hash for security
- Self-hosting the library for better control
| "body": "export default {\n\tgenPDF: () => {\n\t\tconst doc = new jspdf.jsPDF();\n\tdoc.text('Users', 20, 20);\n\t\tdoc.table(20, 30, Table1.tableData, Table1.columnOrder, {autoSize: true});\n\t\tdownload(doc.output(), 'users_list.pdf');\n\t}\n}", | ||
| "variables": [], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PDF generation code lacks error handling
The PDF generation code in JSObject1 doesn't handle potential errors from jsPDF operations.
Consider:
export default {
genPDF: () => {
+ try {
const doc = new jspdf.jsPDF();
doc.text('Users', 20, 20);
doc.table(20, 30, Table1.tableData, Table1.columnOrder, {autoSize: true});
download(doc.output(), 'users_list.pdf');
+ } catch (error) {
+ showAlert('Failed to generate PDF: ' + error.message);
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
| "timeoutInMillisecond": 10000.0, | ||
| "paginationType": "NONE", | ||
| "encodeParamsToggle": true, | ||
| "selfReferencingDataPaths": [], | ||
| "formData": { | ||
| "command": { "data": "FETCH_MANY" }, | ||
| "entityType": { "data": "ROWS" }, | ||
| "tableHeaderIndex": { "data": "1" }, | ||
| "projection": { "data": ["Currency"] }, | ||
| "queryFormat": { "data": "ROWS" }, | ||
| "range": { "data": "" }, | ||
| "where": { | ||
| "data": { | ||
| "condition": "AND", | ||
| "children": [{ "condition": "LT" }] | ||
| } | ||
| }, | ||
| "pagination": { "data": { "limit": "20", "offset": "0" } }, | ||
| "smartSubstitution": { "data": true }, | ||
| "sheetUrl": { | ||
| "data": "https://docs.google.com/spreadsheets/d/1DLNNumIhXhbajHGAEhWfzlgixcAu259gzAOVvLtWVpA/edit" | ||
| }, | ||
| "sheetName": { "data": "Sheet1" }, | ||
| "sortBy": { "data": [{ "column": "", "order": "Ascending" }] } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Potential security risk in Google Sheets API configuration
The Google Sheets API configuration exposes a public sheet URL and has incomplete WHERE clause conditions.
Consider:
-"sheetUrl": {
- "data": "https://docs.google.com/spreadsheets/d/1DLNNumIhXhbajHGAEhWfzlgixcAu259gzAOVvLtWVpA/edit"
-},
+"sheetUrl": {
+ "data": "{{config.GOOGLE_SHEETS_URL}}"
+},📝 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.
| "timeoutInMillisecond": 10000.0, | |
| "paginationType": "NONE", | |
| "encodeParamsToggle": true, | |
| "selfReferencingDataPaths": [], | |
| "formData": { | |
| "command": { "data": "FETCH_MANY" }, | |
| "entityType": { "data": "ROWS" }, | |
| "tableHeaderIndex": { "data": "1" }, | |
| "projection": { "data": ["Currency"] }, | |
| "queryFormat": { "data": "ROWS" }, | |
| "range": { "data": "" }, | |
| "where": { | |
| "data": { | |
| "condition": "AND", | |
| "children": [{ "condition": "LT" }] | |
| } | |
| }, | |
| "pagination": { "data": { "limit": "20", "offset": "0" } }, | |
| "smartSubstitution": { "data": true }, | |
| "sheetUrl": { | |
| "data": "https://docs.google.com/spreadsheets/d/1DLNNumIhXhbajHGAEhWfzlgixcAu259gzAOVvLtWVpA/edit" | |
| }, | |
| "sheetName": { "data": "Sheet1" }, | |
| "sortBy": { "data": [{ "column": "", "order": "Ascending" }] } | |
| } | |
| }, | |
| "timeoutInMillisecond": 10000.0, | |
| "paginationType": "NONE", | |
| "encodeParamsToggle": true, | |
| "selfReferencingDataPaths": [], | |
| "formData": { | |
| "command": { "data": "FETCH_MANY" }, | |
| "entityType": { "data": "ROWS" }, | |
| "tableHeaderIndex": { "data": "1" }, | |
| "projection": { "data": ["Currency"] }, | |
| "queryFormat": { "data": "ROWS" }, | |
| "range": { "data": "" }, | |
| "where": { | |
| "data": { | |
| "condition": "AND", | |
| "children": [{ "condition": "LT" }] | |
| } | |
| }, | |
| "pagination": { "data": { "limit": "20", "offset": "0" } }, | |
| "smartSubstitution": { "data": true }, | |
| "sheetUrl": { | |
| "data": "{{config.GOOGLE_SHEETS_URL}}" | |
| }, | |
| "sheetName": { "data": "Sheet1" }, | |
| "sortBy": { "data": [{ "column": "", "order": "Ascending" }] } | |
| } | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
app/client/cypress/fixtures/PartialImportAppNegative.json (1)
6-6: Consider using a more descriptive application name.The application name "Untitled application 6" appears to be a default or temporary name. Even for test fixtures, using descriptive names helps improve test readability and maintenance.
- "name": "Untitled application 6", + "name": "IncompatibleJsonImportTest",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts(1 hunks)app/client/cypress/fixtures/PartialImportAppNegative.json(1 hunks)app/client/cypress/limited-tests.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/cypress/limited-tests.txt
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts (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.
app/client/cypress/fixtures/PartialImportAppNegative.json (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.
🔇 Additional comments (3)
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts (3)
1-5: LGTM! Well-structured imports following Page Object Model.
The imports are clean and follow best practices by importing specific objects from the core objects file.
7-10: LGTM! Well-structured test suite declaration with proper tagging.
The test suite is properly tagged and has a clear description.
11-13: LGTM! Appropriate use of beforeEach for test setup.
The beforeEach hook is used appropriately to set up the test environment.
| it("1. Verify Importing App into Page shows error message for incompatible json file ", () => { | ||
| homePage.ImportApp("PartialImportAppNegative.json", "", true); | ||
| agHelper.ValidateToastMessage("Unable to import artifact in workspace The file is not compatible with the current partial import operation. Please check the file and try again.. {1}") | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor test case to follow best practices.
- Move the error message to a constant or fixture to avoid hardcoded strings in assertions
- Consider moving the test file name to a constant or fixture
+ const ERROR_MESSAGES = {
+ INCOMPATIBLE_FILE: "Unable to import artifact in workspace The file is not compatible with the current partial import operation. Please check the file and try again.. {1}"
+ };
+ const TEST_FILES = {
+ INCOMPATIBLE_JSON: "PartialImportAppNegative.json"
+ };
it("1. Verify Importing App into Page shows error message for incompatible json file ", () => {
- homePage.ImportApp("PartialImportAppNegative.json", "", true);
- agHelper.ValidateToastMessage("Unable to import artifact in workspace The file is not compatible with the current partial import operation. Please check the file and try again.. {1}")
+ homePage.ImportApp(TEST_FILES.INCOMPATIBLE_JSON, "", true);
+ agHelper.ValidateToastMessage(ERROR_MESSAGES.INCOMPATIBLE_FILE)
});Committable suggestion skipped: line range outside the PR's diff.
| "body": "export default {\n\tmyVar1: [],\n\tmyVar2: {},\n\tmyFun1 () {\n\t\tshowAlert(moment().daysInMonth().toString());\n\t},\n\tasync myFun2 () {\n\t\t//\tuse async-await or promises\n\t\t//\tawait storeValue('varName', 'hello world')\n\t}\n}", | ||
| "variables": [ | ||
| { "name": "myVar1", "value": "[]" }, | ||
| { "name": "myVar2", "value": "{}" } | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused JavaScript object.
JSObject2 appears to be unused and contains only template code with TODO comments.
Consider removing this unused object or implementing the required functionality.
| "data": { | ||
| "condition": "AND", | ||
| "children": [{ "condition": "LT" }] | ||
| } |
There was a problem hiding this comment.
Incomplete WHERE clause condition.
The API query's WHERE clause has an incomplete condition that only specifies "LT" without any column or value.
- "children": [{ "condition": "LT" }]
+ "children": [{
+ "condition": "LT",
+ "key": "Currency",
+ "value": 1000
+ }]📝 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.
| "data": { | |
| "condition": "AND", | |
| "children": [{ "condition": "LT" }] | |
| } | |
| "data": { | |
| "condition": "AND", | |
| "children": [{ | |
| "condition": "LT", | |
| "key": "Currency", | |
| "value": 1000 | |
| }] | |
| } |
| "pluginType": "JS", | ||
| "actions": [], | ||
| "archivedActions": [], | ||
| "body": "export default {\n\tgenPDF: () => {\n\t\tconst doc = new jspdf.jsPDF();\n\tdoc.text('Users', 20, 20);\n\t\tdoc.table(20, 30, Table1.tableData, Table1.columnOrder, {autoSize: true});\n\t\tdownload(doc.output(), 'users_list.pdf');\n\t}\n}", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to PDF generation.
The PDF generation code in JSObject1 lacks try-catch blocks for error handling.
- "body": "export default {\n\tgenPDF: () => {\n\t\tconst doc = new jspdf.jsPDF();\n\tdoc.text('Users', 20, 20);\n\t\tdoc.table(20, 30, Table1.tableData, Table1.columnOrder, {autoSize: true});\n\t\tdownload(doc.output(), 'users_list.pdf');\n\t}\n}",
+ "body": "export default {\n\tgenPDF: () => {\n\t\ttry {\n\t\t\tconst doc = new jspdf.jsPDF();\n\t\t\tdoc.text('Users', 20, 20);\n\t\t\tdoc.table(20, 30, Table1.tableData, Table1.columnOrder, {autoSize: true});\n\t\t\tdownload(doc.output(), 'users_list.pdf');\n\t\t} catch (error) {\n\t\t\tshowAlert('Failed to generate PDF: ' + error.message);\n\t\t}\n\t}\n}",Committable suggestion skipped: line range outside the PR's diff.
| { | ||
| "datasourceConfiguration": { "url": "" }, | ||
| "name": "Untitled datasource 2", | ||
| "pluginId": "google-sheets-plugin", | ||
| "messages": [], | ||
| "isAutoGenerated": false, | ||
| "gitSyncId": "65afeb7c4b703a552d18577d_26991d11-08ef-4141-a3e3-9d78b1a1c260", | ||
| "deleted": false | ||
| } |
There was a problem hiding this comment.
Datasource configuration lacks required URL.
The Google Sheets datasource configuration has an empty URL which could cause connection issues.
- "datasourceConfiguration": { "url": "" },
+ "datasourceConfiguration": {
+ "url": "https://sheets.googleapis.com/v4/spreadsheets"
+ },Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
app/client/cypress/limited-tests.txt (1)
2-2: Consider adding a brief description of the test's purposeTo improve maintainability, consider adding a comment describing what this specific test validates.
Add a comment like:
+# Tests negative scenarios for incompatible JSON file imports cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.tsapp/client/cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts (1)
7-14: Consider using a more specific setup approach.While the setup is generally good, consider if the
beforeEachhook could be replaced with a more specific setup within the test case itself, as per our guidelines about avoiding hooks. However, if this setup is truly required before each test, the current approach is acceptable.app/client/cypress/fixtures/PartialImportAppNegative.json (1)
512-516: Remove unused template codeJSObject2 contains unused template code with TODO comments.
Consider removing this unused object if it's not needed for the test scenario.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts(1 hunks)app/client/cypress/fixtures/PartialImportAppNegative.json(1 hunks)app/client/cypress/limited-tests.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts (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.
app/client/cypress/fixtures/PartialImportAppNegative.json (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.
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.
🔇 Additional comments (3)
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts (2)
1-6: LGTM! Clean and modular imports.
The imports follow good practices by using destructuring and importing from a centralized ObjectsCore.
15-18: 🛠️ Refactor suggestion
Extract error message to a constant and verify selectors.
- The hardcoded error message string should be extracted to a constant for better maintainability and reuse.
- Ensure that
ImportAppinternally uses data-* attributes for selectors.
Apply this refactor:
+ const INCOMPATIBLE_FILE_ERROR = "Unable to import artifact in workspace The file is not compatible with the current partial import operation. Please check the file and try again.. {1}";
it("1. Verify Importing App into Page shows error message for incompatible json file ", () => {
homePage.ImportApp("PartialImportAppNegative.json", "", true);
- agHelper.ValidateToastMessage("Unable to import artifact in workspace The file is not compatible with the current partial import operation. Please check the file and try again.. {1}")
+ agHelper.ValidateToastMessage(INCOMPATIBLE_FILE_ERROR)
});✅ Verification successful
Extract error message to a constant and use data attributes for selectors
The HomePage class implementation shows proper use of data-testid attributes for selectors (e.g., [data-testid=t--workspace-import-app]). However, the error message should be extracted to a constant for maintainability.
+ const INCOMPATIBLE_FILE_ERROR = "Unable to import artifact in workspace The file is not compatible with the current partial import operation. Please check the file and try again.. {1}";
it("1. Verify Importing App into Page shows error message for incompatible json file ", () => {
homePage.ImportApp("PartialImportAppNegative.json", "", true);
- agHelper.ValidateToastMessage("Unable to import artifact in workspace The file is not compatible with the current partial import operation. Please check the file and try again.. {1}")
+ agHelper.ValidateToastMessage(INCOMPATIBLE_FILE_ERROR)
});🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if ImportApp uses proper selectors
ast-grep --pattern 'ImportApp($$$) {
$$$
cy.get($selector)
$$$
}'
Length of output: 73
Script:
#!/bin/bash
# Let's try a different approach to find ImportApp implementation
rg -U "ImportApp.*\{[\s\S]*?\}" --type ts
# Also search for any cy.get usage in the same file
rg "cy\.get" "app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts"
# Look for homePage object definition
rg "class.*HomePage" --type ts
Length of output: 3744
Script:
#!/bin/bash
# Let's check the HomePage class implementation
rg -U "class HomePage[\s\S]*?ImportApp[\s\S]*?}" app/client/cypress/support/Pages/HomePage.ts
# Also check for any constants related to error messages
rg "Unable to import artifact" --type ts
Length of output: 7261
app/client/cypress/fixtures/PartialImportAppNegative.json (1)
2-4: Verify schema version compatibility
The significant gap between client schema version (1.0) and server schema version (11.0) could lead to compatibility issues during import.
app/client/cypress/limited-tests.txt
Outdated
| @@ -1,5 +1,5 @@ | |||
| # To run only limited tests - give the spec names in below format: | |||
| cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js | |||
| cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts | |||
There was a problem hiding this comment.
Fix typo in spec filename: "Parital" → "Partial"
The specification filename contains a typo that should be corrected to ensure consistency and prevent potential issues.
Apply this diff:
-cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts
+cypress/e2e/Regression/ClientSide/PartialImportExport/PartialImportAppNegative_spec.ts📝 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.
| cypress/e2e/Regression/ClientSide/PartialImportExport/ParitalImportAppNegative_spec.ts | |
| cypress/e2e/Regression/ClientSide/PartialImportExport/PartialImportAppNegative_spec.ts |
| "where": { | ||
| "data": { | ||
| "condition": "AND", | ||
| "children": [{ "condition": "LT" }] | ||
| } | ||
| }, |
There was a problem hiding this comment.
Fix incomplete where clause condition
The API where clause contains an incomplete "LT" condition without specifying the column and value.
{
"condition": "AND",
"children": [
- { "condition": "LT" }
+ {
+ "condition": "LT",
+ "key": "Currency",
+ "value": 1000
+ }
]
}📝 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.
| "where": { | |
| "data": { | |
| "condition": "AND", | |
| "children": [{ "condition": "LT" }] | |
| } | |
| }, | |
| "where": { | |
| "data": { | |
| "condition": "AND", | |
| "children": [{ | |
| "condition": "LT", | |
| "key": "Currency", | |
| "value": 1000 | |
| }] | |
| } | |
| }, |
| "datasourceList": [ | ||
| { | ||
| "datasourceConfiguration": { "url": "" }, | ||
| "name": "Untitled datasource 2", | ||
| "pluginId": "google-sheets-plugin", | ||
| "messages": [], | ||
| "isAutoGenerated": false, | ||
| "gitSyncId": "65afeb7c4b703a552d18577d_26991d11-08ef-4141-a3e3-9d78b1a1c260", | ||
| "deleted": false | ||
| } |
There was a problem hiding this comment.
Fix empty datasource URL
The datasource configuration has an empty URL which will cause connection failures.
{
"datasourceConfiguration": {
- "url": ""
+ "url": "https://docs.google.com/spreadsheets/d/<sheet-id>/edit"
}
}Committable suggestion skipped: line range outside the PR's diff.
| "pluginType": "JS", | ||
| "actions": [], | ||
| "archivedActions": [], | ||
| "body": "export default {\n\tgenPDF: () => {\n\t\tconst doc = new jspdf.jsPDF();\n\tdoc.text('Users', 20, 20);\n\t\tdoc.table(20, 30, Table1.tableData, Table1.columnOrder, {autoSize: true});\n\t\tdownload(doc.output(), 'users_list.pdf');\n\t}\n}", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to PDF generation
The PDF generation function lacks error handling which could lead to unhandled runtime exceptions.
export default {
genPDF: () => {
+ try {
const doc = new jspdf.jsPDF();
doc.text('Users', 20, 20);
doc.table(20, 30, Table1.tableData, Table1.columnOrder, {autoSize: true});
download(doc.output(), 'users_list.pdf');
+ } catch (error) {
+ showAlert('Failed to generate PDF: ' + error.message, 'error');
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11783494846. |
NandanAnantharamu
added
ok-to-test
Added test for incompatible json file which validates error message.
/ok-to-test tags="@tag.All"
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11842679143
Commit: e25dece
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ServerSide/ApiTests/API_MultiPart_Spec.ts
List of identified flaky tests.Thu, 14 Nov 2024 19:08:36 UTC
Summary by CodeRabbit
New Features
Bug Fixes