test: updated communityIssues spec by replacing 3rd party APIs #36599
Conversation
NandanAnantharamu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe pull request introduces several modifications primarily focused on the test specifications within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
|
/ci-test-limit-count run_count=25 |
github-actions
bot
added
skip-changelog
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11092986029. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
app/client/cypress/support/Pages/Table.ts (1)
531-531: Well done on updating the CloseFilter method, but let's add some clarity!The change looks good, class! You've updated the
GetNClickmethod call with additional parameters, which should improve its functionality. However, to help your fellow students understand the reasoning behind this change, could you please add a comment explaining why these parameters were necessary? Remember, clear communication is key in coding!Consider adding a comment above this line to explain the purpose of the
0andtrueparameters. For example:+// Added index and force parameters to ensure reliable clicking of the filter close button this.agHelper.GetNClick(this._filterCloseBtn, 0, true);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/cypress/e2e/Regression/Apps/CommunityIssues_Spec.ts (7 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
- app/client/cypress/support/Pages/Table.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/client/cypress/e2e/Regression/Apps/CommunityIssues_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/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.
app/client/cypress/support/Pages/Table.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.
🔇 Additional comments (4)
app/client/cypress/limited-tests.txt (1)
2-2: Well done, class! This change looks good.I'm pleased to see you've updated the test specification from 'Fork_Template_spec.js' to 'CommunityIssues_Spec.ts'. This aligns perfectly with our lesson plan of updating the community specification test steps.
Remember, children, while we're focusing on this particular test today, it's crucial not to forget about our other tests. They're like the other subjects in our curriculum - all important for a well-rounded education!
app/client/cypress/e2e/Regression/Apps/CommunityIssues_Spec.ts (3)
Line range hint
260-271: Let's discuss the changes in our loop iterations, shall we?I've noticed that we've adjusted the number of times we're checking our data:
- On line 260, we've changed from 5 to 4 iterations.
- On line 271, we've changed from 7 to 6 iterations.
Can someone explain to the class why we made these changes? Remember, when we change our experiment parameters, we need to understand and document the reasons. This helps us learn and ensures our tests remain thorough and accurate.
Could you please add a comment explaining the reasoning behind these changes? For example:
// Reduced iterations to 4 because [your reason here] for (let i = 0; i < 4; i++) { // ... } // Reduced iterations to 6 because [your reason here] for (let i = 0; i < 6; i++) { // ... }This way, future students (I mean, developers) will understand our scientific method!
393-397: Class, let's examine our data collection method!I've noticed we've changed which row we're looking at in our table. We used to look at row 0, but now we're checking row 1:
table.ReadTableRowColumnData(1, 0, "v2", 2000).then((cellData) => { // ... }); table.ReadTableRowColumnData(1, 1, "v2").then((cellData) => { // ... });Can someone explain to the class why we've shifted our focus? When we change which data we're examining, we need to understand and document the reasons.
Let's add a comment to explain this change. For example:
// Changed to row 1 because [your reason here] table.ReadTableRowColumnData(1, 0, "v2", 2000).then((cellData) => { // ... });Remember, good scientists always explain their methodology!
Line range hint
1-430: Class, let's summarize our lesson on writing good tests!Today, we've examined several changes to our Community Issues test suite. While we've made some excellent additions, like testing the deletion of issues, we've also found areas where we can improve:
- Remember to break down long assertions into smaller, more manageable parts.
- Always deploy our app consistently in each test to ensure reliable results.
- Document the reasons for changing test parameters, like loop iterations or row indices.
- Use environment variables for configurable values like URLs to make our tests more portable.
- Be careful about commenting out assertions - if they're not needed, remove them; if they are, keep them active.
For homework, I'd like everyone to review their changes and apply these principles. Remember, clear, consistent, and well-documented tests are the foundation of good software development!
Keep up the good work, and don't forget to raise your hand (or open a discussion) if you have any questions!
| // "TypeTitleStatus+1CommentorsVotesAnswerUpVote", | ||
| "TypeTitleStatus+1CommentorsVotesAnswerUpVoteStatesupvote_ididgithub_issue_idauthorcreated_atdescriptionlabelsstatelinkupdated_at", |
There was a problem hiding this comment.
Class, let's break down this long assertion into smaller parts!
Children, while it's commendable that you've updated the assertion to include all columns, we need to make our code more readable. Remember our lesson on keeping things simple?
Let's break this long string into multiple assertions:
table.AssertTableHeaderOrder("Type");
table.AssertTableHeaderOrder("Title");
table.AssertTableHeaderOrder("Status");
// ... and so on for each columnThis way, if a test fails, we'll know exactly which column is causing the issue. It's like solving a big math problem step by step!
| @@ -226,7 +227,7 @@ | |||
| EditorNavigation.SelectEntityByName("Table1", EntityType.Widget); | |||
| propPane.TogglePropertyState("Client side search", "Off"); | |||
|
|
|||
| deployMode.DeployApp(locators._widgetInDeployed(draggableWidgets.TABLE)); | |||
| // deployMode.DeployApp(locators._widgetInDeployed(draggableWidgets.TABLE)); | |||
There was a problem hiding this comment.
Oh dear, why have we skipped a step in our experiment?
Class, I noticed that we've commented out the app deployment step. This is like trying to bake a cake without preheating the oven! We need to ensure our test environment is consistent for every experiment.
Let's uncomment this line:
deployMode.DeployApp(locators._widgetInDeployed(draggableWidgets.TABLE));Remember, in science (and in testing), we need to follow all the steps in our procedure to get accurate results!
| @@ -309,7 +310,7 @@ | |||
| .type("Adding Description Suggestion via script"); | |||
| cy.get(locators._inputWidgetv1InDeployed) | |||
| .eq(4) | |||
| .type("https://github.com/appsmithorg/appsmith/issues/12532"); | |||
| .type("http://host.docker.internal:5001"); | |||
There was a problem hiding this comment.
Attention class! We need to discuss our choice of URL.
I see we've changed our URL to http://host.docker.internal:5001. While this might work on our local machines, it's like assuming everyone has the same textbook as you. What happens when someone tries to run this test on a different computer?
Let's make our experiment more adaptable. We could use an environment variable or a configuration file to set this URL. For example:
.type(Cypress.env('TEST_URL') || 'http://localhost:5001');This way, we can easily change the URL for different environments without modifying our test code. It's like having a variable in our equation that we can easily adjust!
Remember, good scientists make their experiments reproducible in any lab!
| // agHelper.AssertElementAbsence(locators._widgetInDeployed("tabswidget")); | ||
| table.SelectTableRow(0, 1, true, "v2"); | ||
| agHelper.AssertElementVisibility( |
There was a problem hiding this comment.
Let's talk about our observation skills, class!
I've noticed that we've put on blinders for some of our checks:
// agHelper.AssertElementAbsence(locators._widgetInDeployed("tabswidget"));And then we're immediately checking if it's visible:
agHelper.AssertElementVisibility(
locators._widgetInDeployed("tabswidget"),
);This is like closing our eyes and then trying to see if something is there! We need to decide what we're actually testing for.
If the first assertion is no longer needed, let's remove it entirely. If it's still important, we should uncomment it. Remember, in science, every observation is important!
Let's clean this up to make our experiment clear and concise. What are we really trying to test here?
| // agHelper.AssertElementAbsence(locators._widgetInDeployed("tabswidget")); | ||
| agHelper.GetNClick(".cross-icon"); | ||
| table.SearchTable("Suggestion"); | ||
| table.WaitUntilTableLoad(0, 0, "v2"); | ||
| table.SelectTableRow(0, 0, true, "v2"); | ||
| agHelper.AssertElementVisibility( | ||
| locators._widgetInDeployed("tabswidget"), | ||
| ); | ||
| cy.get(table._trashIcon).closest("div").click({ force: true }); | ||
| agHelper.WaitUntilEleDisappear(locators._widgetInDeployed("tabswidget")); | ||
| agHelper.AssertElementAbsence(locators._widgetInDeployed("tabswidget")); | ||
| // agHelper.WaitUntilEleDisappear(locators._widgetInDeployed("tabswidget")); | ||
| // agHelper.AssertElementAbsence(locators._widgetInDeployed("tabswidget")); |
There was a problem hiding this comment.
Excellent addition to our experiment, class! But let's tidy up a bit.
I'm thrilled to see we've added steps to test deleting a newly created issue. This is like checking if we can erase what we've written - very important!
However, I've noticed we've left some of our checkpoints commented out:
// agHelper.WaitUntilEleDisappear(locators._widgetInDeployed("tabswidget"));
// agHelper.AssertElementAbsence(locators._widgetInDeployed("tabswidget"));Remember, in science, we need to be thorough with our observations. If these checks are no longer needed, let's remove them entirely. If they're still important, we should uncomment them.
Let's decide which observations are crucial for our experiment and keep our lab notebook (code) clean and clear. What do you think, class? Which checks do we need to keep?
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11092986029. |
|
/ci-test-limit-count run_count=25 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11103147911. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11103147911. |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11160776203. |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11163194519. |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11166936572. |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11178644049. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11178644049. |
|
/ci-test-limit-count run_count=25 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11179028326. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11179028326. |
|
/ci-test-limit-count run_count=25 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11209296033. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11209296033. |
reverted limited file
NandanAnantharamu
added
ok-to-test
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
updated test steps in community spec and common method.
/ok-to-test tags="@tag.All"
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11288210535
Commit: cfea07d
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Google/EnableGoogle_spec.js
List of identified flaky tests.Fri, 11 Oct 2024 11:01:57 UTC
Summary by CodeRabbit
Bug Fixes
New Features
Documentation