-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: executing skipped tests on CE #36681
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -9,6 +9,7 @@ import { | |||||||||||||||||||
homePage, | ||||||||||||||||||||
locators, | ||||||||||||||||||||
table, | ||||||||||||||||||||
propPane, | ||||||||||||||||||||
} from "../../../../support/Objects/ObjectsCore"; | ||||||||||||||||||||
import EditorNavigation, { | ||||||||||||||||||||
AppSidebar, | ||||||||||||||||||||
|
@@ -221,46 +222,46 @@ describe( | |||||||||||||||||||
}); | ||||||||||||||||||||
|
||||||||||||||||||||
//Open Bug 14063 - hence skipping | ||||||||||||||||||||
// it.skip("6. Verify Update/Delete row/Delete field data from Deploy page - on Productlines - existing record + Bug 14063", () => { | ||||||||||||||||||||
// EditorNavigation.SelectEntityByName("update_form", EntityType.Widget); | ||||||||||||||||||||
// propPane.ChangeJsonFormFieldType( | ||||||||||||||||||||
// "Text Description", | ||||||||||||||||||||
// "Multiline Text Input", | ||||||||||||||||||||
// ); | ||||||||||||||||||||
// propPane.NavigateBackToPropertyPane(); | ||||||||||||||||||||
// propPane.ChangeJsonFormFieldType( | ||||||||||||||||||||
// "Html Description", | ||||||||||||||||||||
// "Multiline Text Input", | ||||||||||||||||||||
// ); | ||||||||||||||||||||
// propPane.NavigateBackToPropertyPane(); | ||||||||||||||||||||
// deployMode.DeployApp(); | ||||||||||||||||||||
// table.SelectTableRow(0, 0, false); //to make JSON form hidden | ||||||||||||||||||||
// agHelper.AssertElementAbsence(locators._jsonFormWidget); | ||||||||||||||||||||
// table.SelectTableRow(3); | ||||||||||||||||||||
// agHelper.AssertElementVisibility(locators._jsonFormWidget); | ||||||||||||||||||||
|
||||||||||||||||||||
// dataSources.AssertJSONFormHeader(3, 0, "productLine"); | ||||||||||||||||||||
|
||||||||||||||||||||
// deployMode.EnterJSONTextAreaValue( | ||||||||||||||||||||
// "Html Description", | ||||||||||||||||||||
// "The largest cruise ship is twice the length of the Washington Monument. Some cruise ships have virtual balconies.", | ||||||||||||||||||||
// ); | ||||||||||||||||||||
// agHelper.ClickButton("Update"); //Update does not work, Bug 14063 | ||||||||||||||||||||
// agHelper.AssertElementAbsence(locators._toastMsg); //Validating fix for Bug 14063 | ||||||||||||||||||||
// assertHelper.AssertNetworkStatus("@postExecute", 200); | ||||||||||||||||||||
// table.AssertSelectedRow(3); | ||||||||||||||||||||
|
||||||||||||||||||||
// //validating update happened fine! | ||||||||||||||||||||
// table.ReadTableRowColumnData(3, 2, "v1", 200).then(($cellData) => { | ||||||||||||||||||||
// expect($cellData).to.eq( | ||||||||||||||||||||
// "The largest cruise ship is twice the length of the Washington Monument. Some cruise ships have virtual balconies.", | ||||||||||||||||||||
// ); | ||||||||||||||||||||
// }); | ||||||||||||||||||||
// }); | ||||||||||||||||||||
it("6. Verify Update/Delete row/Delete field data from Deploy page - on Productlines - existing record + Bug 14063", () => { | ||||||||||||||||||||
EditorNavigation.SelectEntityByName("update_form", EntityType.Widget); | ||||||||||||||||||||
propPane.ChangeJsonFormFieldType( | ||||||||||||||||||||
"Text Description", | ||||||||||||||||||||
"Multiline Text Input", | ||||||||||||||||||||
); | ||||||||||||||||||||
propPane.NavigateBackToPropertyPane(); | ||||||||||||||||||||
propPane.ChangeJsonFormFieldType( | ||||||||||||||||||||
"Html Description", | ||||||||||||||||||||
"Multiline Text Input", | ||||||||||||||||||||
); | ||||||||||||||||||||
Comment on lines
+227
to
+235
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace plain strings with locator variables for better maintainability To adhere to best practices, please use locator variables instead of plain strings when specifying field names and button labels. This applies to methods like For example, you might define locator variables such as - propPane.ChangeJsonFormFieldType(
- "Text Description",
- "Multiline Text Input",
- );
+ propPane.ChangeJsonFormFieldType(
+ this.locators.textDescriptionField,
+ "Multiline Text Input",
+ );
...
- agHelper.ClickButton("Update");
+ agHelper.ClickButton(this.locators.updateButton); Also applies to: 245-249 |
||||||||||||||||||||
propPane.NavigateBackToPropertyPane(); | ||||||||||||||||||||
deployMode.DeployApp(); | ||||||||||||||||||||
Comment on lines
+225
to
+237
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider using locator variables instead of plain strings for selectors In your test steps where you change the JSON form field types, you're using plain strings like To improve your code, you might define constants for these field names: const FIELD_TEXT_DESCRIPTION = "Text Description";
const FIELD_MULTILINE_TEXT_INPUT = "Multiline Text Input";
propPane.ChangeJsonFormFieldType(FIELD_TEXT_DESCRIPTION, FIELD_MULTILINE_TEXT_INPUT); |
||||||||||||||||||||
table.SelectTableRow(0, 0, false); //to make JSON form hidden | ||||||||||||||||||||
agHelper.AssertElementAbsence(locators._jsonFormWidget); | ||||||||||||||||||||
table.SelectTableRow(3); | ||||||||||||||||||||
agHelper.AssertElementVisibility(locators._jsonFormWidget); | ||||||||||||||||||||
|
||||||||||||||||||||
dataSources.AssertJSONFormHeader(3, 0, "productLine"); | ||||||||||||||||||||
|
||||||||||||||||||||
deployMode.EnterJSONTextAreaValue( | ||||||||||||||||||||
"Html Description", | ||||||||||||||||||||
"The largest cruise ship is twice the length of the Washington Monument. Some cruise ships have virtual balconies.", | ||||||||||||||||||||
); | ||||||||||||||||||||
agHelper.ClickButton("Update"); //Update does not work, Bug 14063 | ||||||||||||||||||||
Comment on lines
+245
to
+249
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use locator variables instead of plain strings when interacting with UI elements In lines where you interact with UI elements, such as Consider defining locator variables for these elements: const FIELD_HTML_DESCRIPTION = "Html Description";
const BUTTON_UPDATE = locators._updateButton; // Assuming this locator exists
deployMode.EnterJSONTextAreaValue(FIELD_HTML_DESCRIPTION, "Your text here");
agHelper.ClickButton(BUTTON_UPDATE); |
||||||||||||||||||||
agHelper.AssertElementAbsence(locators._toastMsg); //Validating fix for Bug 14063 | ||||||||||||||||||||
assertHelper.AssertNetworkStatus("@postExecute", 200); | ||||||||||||||||||||
table.AssertSelectedRow(3); | ||||||||||||||||||||
|
||||||||||||||||||||
// it.skip("7. Verify Add/Update/Delete from Deploy page - on Productlines - new record + Bug 14063", () => { | ||||||||||||||||||||
// //To script aft bug fix! | ||||||||||||||||||||
// }); | ||||||||||||||||||||
//validating update happened fine! | ||||||||||||||||||||
table.ReadTableRowColumnData(3, 2, "v1", 200).then(($cellData) => { | ||||||||||||||||||||
expect($cellData).to.eq( | ||||||||||||||||||||
"The largest cruise ship is twice the length of the Washington Monument. Some cruise ships have virtual balconies.", | ||||||||||||||||||||
); | ||||||||||||||||||||
}); | ||||||||||||||||||||
Comment on lines
+255
to
+259
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Avoid hardcoding strings in assertions It's advisable to store expected values in variables when performing assertions with long strings. This improves clarity and makes future modifications easier. Here's how you can refactor the assertion: - expect($cellData).to.eq(
- "The largest cruise ship is twice the length of the Washington Monument. Some cruise ships have virtual balconies.",
- );
+ const expectedDescription = "The largest cruise ship is twice the length of the Washington Monument. Some cruise ships have virtual balconies.";
+ expect($cellData).to.eq(expectedDescription); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||
}); | ||||||||||||||||||||
|
||||||||||||||||||||
it("7. Verify Add/Update/Delete from Deploy page - on Productlines - new record + Bug 14063", () => { | ||||||||||||||||||||
//To script aft bug fix! | ||||||||||||||||||||
}); | ||||||||||||||||||||
|
||||||||||||||||||||
it("8. Validate Deletion of the Newly Created Page - Productlines", () => { | ||||||||||||||||||||
deployMode.NavigateBacktoEditor(); | ||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -190,7 +190,7 @@ describe( | |
}); | ||
|
||
// https://github.com/appsmithorg/appsmith/issues/29870 Once this issue is fixed then this test case should be enabled and fixed for the table v2 | ||
it.skip("7. Verify Refresh table from Deploy page - on Stores & verify all updates persists : Skipped till #29870 gets fixed", () => { | ||
it("7. Verify Refresh table from Deploy page - on Stores & verify all updates persists : Skipped till #29870 gets fixed", () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Please keep the test skipped until issue #29870 is resolved. Activating the test while the related issue is still open may lead to false negatives and unreliable test results. Once the issue is closed, you can consider enabling the test to ensure it functions as expected. 🔗 Analysis chainClass, let's examine this change carefully. The test case that was previously skipped has now been activated. This is a positive step towards improving our test coverage. However, we need to ensure that the issue mentioned in the comment (#29870) has indeed been resolved. To verify if the issue has been resolved, let's run the following script: If the issue is closed, we can proceed with confidence. If it's still open, we should consider whether it's appropriate to run this test or if we need to make any adjustments. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if issue #29870 has been closed
gh issue view 29870 --json state -q .state
Length of output: 49 |
||
agHelper.GetNClick(dataSources._refreshIcon); | ||
|
||
//Store Address deletion remains | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,18 @@ | ||
# To run only limited tests - give the spec names in below format: | ||
cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js | ||
|
||
cypress/e2e/Sanity/Datasources/MsSQL_Basic_Spec.ts | ||
cypress/e2e/Regression/ClientSide/Autocomplete/JS_AC2_spec.ts | ||
cypress/e2e/Regression/ClientSide/Binding/DatePicker_Text_spec.js | ||
cypress/e2e/Regression/ClientSide/Binding/SelectWidget_Spec.ts | ||
cypress/e2e/Regression/ClientSide/BugTests/DatasourceSchema_spec.ts | ||
cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncedApps_spec.js | ||
cypress/e2e/Regression/ClientSide/Git/GitSync/SwitchBranches_spec.js | ||
cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker_With_Switch_spec.js | ||
cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js | ||
cypress/e2e/Regression/ClientSide/Widgets/Multiselect/MultiSelect5_spec.ts | ||
cypress/e2e/Regression/ClientSide/Widgets/Tab/Tabs_2_spec.ts | ||
cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.ts | ||
cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts | ||
cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelectorCreateShouldBind_spec.ts | ||
Comment on lines
+2
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Attention Team: Missing It looks like the Action Items:
🔗 Analysis chainClass, let's review the changes in our test suite! Good morning, students! Today, we're going to examine the modifications made to our limited-tests.txt file. Let's break it down:
Homework assignment: Let's verify if the 'Fork_Template_spec.js' test is covered elsewhere. Run this command: Any volunteers to share their findings tomorrow? Great effort, everyone! It seems our previous search didn't find the Alright class, our previous search didn’t find the 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any test files related to template forking
rg -t js -t ts "Fork.*Template" "cypress/e2e"
Length of output: 97 Script: # Search for any test files related to template forking across the entire repository
rg -t js -t ts "Fork.*Template"
Length of output: 8175 Script: # Search for any test files related to template forking across the entire repository
rg -t js -t ts "Fork.*Template"
Length of output: 8175 |
||
# For running all specs - uncomment below: | ||
#cypress/e2e/**/**/* | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Class, let's review this test case together!
Excellent work on re-enabling this important test case! It's like bringing a sleeping student back to life in our classroom. However, there are a few areas where we can make our test even better. Let's go through them:
Instead of using plain strings in our expect statements, let's create constants. This will make our test more maintainable and easier to read.
Let's add some comments to explain what each step is doing. This will help future students (I mean, developers) understand our test better.
We're using the same pattern of selecting a row and checking the dropdown value multiple times. We could create a helper function to make our test more DRY (Don't Repeat Yourself).
These changes will make our test case cleaner, more readable, and easier to maintain. Remember, clean code is happy code!
Would you like me to provide a fully refactored version of this test case?