chore: Updated another set of third party urls #36571
Conversation
WalkthroughThis pull request introduces updates to the Cypress test suite, specifically targeting the testing of fetch calls in server-side JavaScript functions. The changes include the implementation of tests that verify cookie handling in fetch requests, introduce a delay mechanism for API calls, and modify the onClick event of a button widget to fetch data from a local mock API instead of a placeholder. Additionally, the limited tests specification has been updated to reflect these changes. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/client/cypress/e2e/Regression/ServerSide/ApiTests/Api_XSS_Vulnerability_spec.ts (2)
Line range hint
18-18: Let's improve our selector, shall we?Now, class, can anyone spot what's wrong with our selector here? That's right, we're using a class selector
.xss-container. Remember, we should always use data attributes for our selectors. It's like labeling our books with special stickers instead of just looking at their color. Let's fix this together!Here's your homework assignment. Replace the class selector with a data attribute:
-agHelper.AssertElementAbsence(".xss-container"); +agHelper.AssertElementAbsence("[data-cy=xss-container]");And don't forget to update the XSS payload to use this new attribute:
-`<img src=x onerror='fetch("/api/v1/admin/env").then(r=>r.text()).then(body=>document.body.insertAdjacentHTML("beforeend", "<h1 class=\"xss-container\" style=\"color:red;font-size:72px;position:absolute;top:0;z-index:9\">Poof!</h1>"))'>`, +`<img src=x onerror='fetch("/api/v1/admin/env").then(r=>r.text()).then(body=>document.body.insertAdjacentHTML("beforeend", "<h1 data-cy=\"xss-container\" style=\"color:red;font-size:72px;position:absolute;top:0;z-index:9\">Poof!</h1>"))'>`,
Line range hint
1-21: Let's review our test structure, class!Now, students, let's take a moment to look at the overall structure of our test. Can anyone tell me what's missing? That's right, we're not following some of our best practices:
- We're not using locator variables for our selectors.
- We only have one assertion in our test.
- We're using a string for our assertion.
Let's work on improving these areas. Who wants to suggest how we can make our test even better?
Here's a homework assignment for you all. Try refactoring the test to address these points:
- Create locator variables for selectors.
- Add more assertions to thoroughly test the XSS vulnerability.
- Use constants instead of strings for assertions.
Remember, practice makes perfect!
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Url_Column_spec.ts (1)
32-32: Another excellent URL update, but let's consider a small enhancement!Well done on consistently updating our URLs to use local resources. This change, like the previous one, will contribute to a more stable testing environment. However, I have a small suggestion to make this even better:
Consider extracting these URLs into constants at the top of the file. This would make it easier to manage and update these URLs in the future if needed. Here's an example of how you could do this:
const PROFILE_PIC_URL_1 = "http://host.docker.internal:4200/453-200x300.jpg"; const PROFILE_PIC_URL_2 = "http://host.docker.internal:4200/photo-1503469432756-4aae2e18d881.jpeg";Then you can use these constants in your
AssertURLColumnNavigationcalls. This approach would make our code more maintainable and easier to read. What do you think about this suggestion?app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/table_data_change_spec.ts (1)
98-98: Good job on consistency, but let's think about realism, shall we?While I appreciate your diligence in updating all the avatar URLs, I noticed that you've used the same specific image (453-200x300.jpg) for multiple entries. In a real-world scenario, it's more likely that users would have unique avatars or the default image. Perhaps we could consider using unique images for each entry or falling back to the default image? This would make our test data even more representative of a real user base. What do you think about this suggestion?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Url_Column_spec.ts (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/table_data_change_spec.ts (4 hunks)
- app/client/cypress/e2e/Regression/ServerSide/ApiTests/Api_XSS_Vulnerability_spec.ts (1 hunks)
- app/client/cypress/fixtures/tableV2WithUrlColumnDsl.json (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Url_Column_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/e2e/Regression/ClientSide/Widgets/TableV2/table_data_change_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/e2e/Regression/ServerSide/ApiTests/Api_XSS_Vulnerability_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/tableV2WithUrlColumnDsl.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 (9)
app/client/cypress/e2e/Regression/ServerSide/ApiTests/Api_XSS_Vulnerability_spec.ts (1)
7-8: Class, let's discuss the change in our API endpoint.Now, children, we've made an important change to our test. We've updated our API endpoint from a public one to a local mock API. This is like changing from a public library to our own classroom library. Can anyone tell me why this might be beneficial for our tests?
Let's verify if this new endpoint is being used consistently across our project. Run this little exercise:
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Url_Column_spec.ts (2)
26-26: Good job updating the URL to use a local resource!Class, let's take a moment to appreciate this change. By updating the URL from an external source to a local Docker host, we're creating a more controlled test environment. This is an excellent practice that will make our tests more reliable and less dependent on external resources. Well done!
Line range hint
1-38: Overall, these changes enhance our test reliability. Great work!Class, let's recap what we've learned from these changes:
- We've successfully updated our URLs to use local resources, which is a best practice in e2e testing.
- The test's purpose and functionality remain unchanged, ensuring we're still effectively testing the TableV2 URL column navigation.
- These updates make our tests more reliable and less dependent on external factors.
Remember, in the world of testing, consistency and control are key. By using local resources, we're taking a step towards more predictable and manageable tests. Keep up the good work!
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/table_data_change_spec.ts (4)
65-65: Excellent work updating the avatar URL, class!I'm pleased to see you've updated the avatar URL to point to our local development environment. This change will help us have more control over our test data and reduce dependencies on external services. Remember, children, in the real world, we always want to minimize external dependencies in our tests!
76-76: Well done on updating another avatar URL, students!I see you've consistently applied the URL change to another avatar. It's worth noting that this image has a specific filename (453-200x300.jpg), which suggests it might represent a user-uploaded avatar. This attention to detail in our test data is commendable! It helps us simulate real-world scenarios more accurately.
87-87: Another gold star for consistency, class!I'm impressed to see you've applied the same clouddefaultImage.png for this avatar as well. This is a great example of efficient test data management. By reusing the default image, we're simulating a common real-world scenario where multiple users might have the default avatar. Keep up the good work!
Line range hint
65-98: Overall, a job well done, class!You've successfully updated all the avatar URLs to use our local development environment. This change will make our tests more reliable and easier to manage. Remember, in software development, just like in this classroom, attention to detail and consistency are key!
However, let's not forget about realism in our test data. Consider varying the avatar images or using the default image more frequently to better simulate a real user base. This small improvement could make our tests even more robust and representative of real-world scenarios.
Keep up the excellent work, and don't hesitate to ask if you have any questions!
app/client/cypress/fixtures/tableV2WithUrlColumnDsl.json (2)
Line range hint
1-501: Excellent work, class! Let's summarize our review.You've done a commendable job updating the image URLs in our table data. This change will make our e2e tests more reliable and less dependent on external services. Here's a quick recap:
- All image URLs now point to our local development environment.
- The change is consistent across all 10 objects in the array.
- The JSON structure remains intact and valid.
Remember, when working with test data, it's crucial to have control over all aspects of the environment. This change exemplifies that principle beautifully.
For your homework, I want you to think about other areas in our codebase where we might apply similar changes to improve our testing process. Class dismissed!
501-501: Class, let's examine the changes made to our table data.Good work on updating the image URLs! I see you've changed them from external sources to our local development environment. This is an excellent practice for our e2e tests, as it reduces dependencies on external services.
Let's break down what you've done:
- All image URLs now point to
http://host.docker.internal:4200.- The file paths after the host have been simplified (e.g.,
/453-200x300.jpgor/photo-1503469432756-4aae2e18d881.jpeg).Can you explain to the class why we're using
host.docker.internalinstead oflocalhost? This would be a great learning opportunity for everyone.To ensure consistency, let's run a quick check:
✅ Verification successful
Great effort on implementing the URL updates! To ensure everything is functioning as expected, let's take a closer look at all the image URLs. Please run the following script to list each image URL for manual verification:
Excellent job! All image URLs have been correctly updated to use
http://host.docker.internal:4200.This ensures our end-to-end tests are reliable and independent of external services.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all image URLs in the tableData # Test: Check if all image URLs start with the expected prefix jq -r '.dsl.children[0].tableData | fromjson | .[].image' app/client/cypress/fixtures/tableV2WithUrlColumnDsl.json | grep -v '^http://host.docker.internal:4200/' # If this command produces no output, all URLs are correctLength of output: 163
Script:
#!/bin/bash # Description: Extract and list all image URLs in the tableData jq -r '.dsl.children[0].tableData | fromjson | .[].image' app/client/cypress/fixtures/tableV2WithUrlColumnDsl.jsonLength of output: 716
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
app/client/cypress/fixtures/onPageLoadActionsDsl.json (2)
37-37: Good job updating the image URL, but let's consider a more appropriate test image.Class, I'm pleased to see that you've updated the
defaultImageURL from a local Docker resource to a public one. This is a step in the right direction for our e2e tests. However, I'd like you to think about whether a favicon is the most suitable image for our testing purposes. Perhaps we could use a more standard test image that better represents what we'd expect in a real scenario?Consider using a more appropriate test image URL, such as a placeholder image service. For example:
- "defaultImage": "https://www.appsmith.com/favicon.png", + "defaultImage": "https://via.placeholder.com/150",This will provide a more realistic image for our tests while still maintaining the benefits of using a public URL.
125-125: Consistency is key, but let's aim for a more suitable test image here as well.I see you've applied the same change to
Image2, which shows good consistency in your work. However, as we discussed withImage1, we should consider using a more appropriate test image here too.Let's apply the same improvement we suggested for
Image1:- "defaultImage": "https://www.appsmith.com/favicon.png", + "defaultImage": "https://via.placeholder.com/150",Remember, class, in our e2e tests, we want to simulate real-world scenarios as closely as possible. Using a proper placeholder image will help us achieve that goal.
app/client/cypress/e2e/Sanity/Datasources/MsSQL_Basic_Spec.ts (2)
Line range hint
95-130: Avoid usingagHelper.Sleep()to improve test efficiency and reliabilityUsing
agHelper.Sleep()introduces arbitrary wait times, which can slow down your tests and make them flaky. Instead, leverage Cypress's built-in commands to wait for specific elements or conditions. This ensures your tests are both efficient and reliable.Here is how you can refactor the code:
- agHelper.Sleep(2000); + agHelper.GetNClick(oneClickBindingLocator.connectData); + assertHelper.AssertNetworkStatus("@postExecute");Replace the fixed delay with commands that wait for specific actions or network responses to complete.
Line range hint
70-80: Address theTODOcomment and restore the skipped testIt's important to resolve
TODOcomments promptly to maintain test coverage. The skipped test indicates there's an issue that needs attention. Instead of skipping the test, consider debugging theInvalid Object <tablename>error. If immediate resolution isn't possible, document the issue in your tracking system.Would you like assistance in debugging this test case? I can help identify the root cause and suggest a solution. Alternatively, should I open a new GitHub issue to ensure this task is tracked?
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/OnLoadActions_Spec.ts (1)
Line range hint
153-158: Refrain from hardcoding URLs; use environment variablesHardcoding the URL
"http://host.docker.internal:5001/v1/genderize/sampledata?name={{RandomUser.data[0].name}}"can make our tests less flexible. It's advisable to use environment variables or configuration files for URLs to enhance test portability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/OnLoadActions_Spec.ts (7 hunks)
- app/client/cypress/e2e/Sanity/Datasources/MsSQL_Basic_Spec.ts (1 hunks)
- app/client/cypress/fixtures/onPageLoadActionsDsl.json (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/OnLoadActions_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/e2e/Sanity/Datasources/MsSQL_Basic_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/onPageLoadActionsDsl.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 (2)
app/client/cypress/fixtures/onPageLoadActionsDsl.json (1)
Line range hint
1-141: Overall assessment: Good progress, but room for improvementClass, I'm pleased with the effort you've put into updating this configuration file. You've successfully moved away from local Docker resources to public URLs, which is a significant step in the right direction for our e2e tests. This change will make our tests more robust and less dependent on local setups.
However, I want you to remember that in testing, we should always strive to simulate real-world scenarios as closely as possible. While using the Appsmith favicon is certainly better than a local resource, it's not the most representative choice for a default image in our tests.
For your homework, I'd like you to:
- Replace both instances of the favicon URL with a more suitable placeholder image URL.
- Consider adding a comment in the JSON file explaining why we're using these placeholder images.
- Think about how we might vary the image sizes or contents to test different scenarios.
Keep up the good work, and don't forget: in the world of testing, attention to detail is our best friend!
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/OnLoadActions_Spec.ts (1)
129-136: Good job on using assertions to validate API namesYour use of assertions to verify the API names ensures that the correct actions are loaded on page load. This is a solid practice that enhances test reliability.
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/OnLoadActions_Spec.ts
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/OnLoadActions_Spec.ts
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/OnLoadActions_Spec.ts
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/OnLoadActions_Spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_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/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (18)
app/client/cypress/e2e/Regression/ClientSide/Widgets/DocumentViewer/DocumentViewer_spec.ts (1)
Line range hint
1-265: Time for a pop quiz on our coding guidelines, class!I've been reviewing your test suite, and while you've done a commendable job overall, there are a few areas where we need to revisit our coding guidelines. Let's go through them together:
I spotted some
cy.waitcommands lurking in our code. Remember, waiting for specific times is like watching the clock in class - it doesn't guarantee the lesson is over! Let's replace these with more intelligent waiting strategies, likecy.intercept()orcy.wait('@alias').I see some hardcoded selectors like
.btn.submitorbutton[type=submit]. These are like using nicknames instead of proper names - they might change! Let's use data-* attributes instead. For example:cy.get('[data-cy="submit-button"]').click()We're using
afterEachhooks, which we agreed to avoid. It's like cleaning up after every question on a test - sometimes it's better to clean up all at once at the end!Here's your homework:
- Replace all
cy.waitcommands with appropriate alternatives.- Update selectors to use data-* attributes.
- Refactor the tests to remove
afterEachhooks, moving necessary cleanup toafterblocks or individual test cases.Remember, following these guidelines is like following the rules in class - it keeps everything running smoothly and fairly for everyone!
app/client/cypress/fixtures/modalWidgetTestApp.json (1)
Line range hint
4-4: Attention, students! We have a homework assignment here.This TODO comment reminds us to add tests for our function. It's crucial to ensure our code works as expected.
Would you like me to create a test case for this function? I can prepare a sample unit test to get you started.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV1/TableBugs_Spec.ts (16)
34-34: Avoid hardcoding expected values in assertionsIt's important to avoid hardcoding strings in your assertions like
expect($cellData).contains("cube-logo_S50__hLNq.jpeg");. This practice can make tests fragile if the data changes. Consider using variables or constants to store expected values for better maintainability.
38-40: Use variables for expected URLs in assertionsDirectly hardcoding URLs in assertions can lead to maintenance issues. Replace the hardcoded URL with a variable or a fixture. This way, if the URL changes, you only need to update it in one place.
44-44: Avoid hardcoding expected values in assertionsAgain, to keep your tests robust, avoid hardcoding expected values like
expect($cellData).contains("zapier-logo_odZ9wZQ3vY.jpeg");. Using variables or test data fixtures can help make your tests more reliable.
48-50: Replace hardcoded URLs with variablesIn the assertion
expect($cellData).to.eq("https://docs.appsmith.com/img/replyto-logo_6yaZHFIeU.jpeg");, it's better to use a variable for the URL. This improves the maintainability of your tests.
56-56: Ensure commented-out code is necessaryI notice that some code is commented out:
// _.table.AssertURLColumnNavigation( // 3, // 0, // "https://docs.appsmith.com/img/replyto-logo_6yaZHFIeU.jpeg", // );If this code is no longer needed, consider removing it to keep the codebase clean.
79-79: Avoid hardcoding expected values in assertionsUsing hardcoded strings in assertions like
expect($cellData).contains("cube-logo_S50__hLNq.jpeg");can make tests less flexible. It's better to use variables to store expected values.
83-85: Use variables for expected URLs in assertionsHardcoding URLs in your tests can cause maintenance challenges. Replace the URL with a variable or reference from a fixture to enhance test reliability.
89-89: Avoid hardcoding expected values in assertionsTo improve test resilience, avoid hardcoding expected values. Use variables instead for
expect($cellData).contains("zapier-logo_odZ9wZQ3vY.jpeg");.
93-95: Replace hardcoded URLs with variablesIn your assertion, consider using a variable for the URL rather than hardcoding it. This approach makes your tests easier to maintain.
101-101: Review commented-out code for necessityThere's commented-out code here:
// _.table.AssertURLColumnNavigation( // 2, // 0, // "https://docs.appsmith.com/img/zapier-logo_odZ9wZQ3vY.jpeg", // );If this code is obsolete, consider removing it to keep the code clean.
124-124: Avoid hardcoding expected values in assertionsUsing hardcoded strings like
expect($cellData).contains("cube-logo_S50__hLNq.jpeg");can lead to brittle tests. Consider storing expected values in variables.
128-130: Use variables for expected URLs in assertionsDirectly hardcoding URLs can make maintenance difficult. Use a variable or fixture to store the URL in your assertion.
134-134: Avoid hardcoding expected values in assertionsFor better test reliability, avoid hardcoding values in assertions like
expect($cellData).contains("zapier-logo_odZ9wZQ3vY.jpeg");. Using variables is a better practice.
138-140: Replace hardcoded URLs with variablesConsider using a variable for the URL in your assertion to improve maintainability.
146-146: Check necessity of commented-out codeThere's commented-out code present:
// _.table.AssertURLColumnNavigation( // 3, // 0, // "https://docs.appsmith.com/img/replyto-logo_6yaZHFIeU.jpeg", // );If it's not needed, it's best to remove it to keep the codebase clean.
Line range hint
181-181: Avoid usingcy.wait()in testsUsing
cy.wait(500);is discouraged as it can lead to flaky tests. Instead, you should wait for specific elements or network calls to complete usingcy.wait()with aliases orcy.get()with assertions.Apply this change to replace
cy.wait(500);:- cy.wait(500); + // Wait for the table data to load + _.agHelper.WaitUntilEleAppear(".t--widget-tablewidgetv1");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/DocumentViewer/DocumentViewer_spec.ts (2 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Modal/Modal_spec.ts (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV1/TableBugs_Spec.ts (3 hunks)
- app/client/cypress/fixtures/TestDataSet1.json (1 hunks)
- app/client/cypress/fixtures/modalWidgetTestApp.json (4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
app/client/cypress/e2e/Regression/ClientSide/Widgets/DocumentViewer/DocumentViewer_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/e2e/Regression/ClientSide/Widgets/Modal/Modal_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/e2e/Regression/ClientSide/Widgets/TableV1/TableBugs_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/TestDataSet1.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/fixtures/modalWidgetTestApp.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 (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/DocumentViewer/DocumentViewer_spec.ts (1)
19-21: Good job updating the image URLs, class!I see you've made some changes to our image URLs. Using the Appsmith favicon for our PNG test is a smart move. It's like bringing your own pencil to the test - always reliable! And that tiny tweak to the JPG URL? Sharp eyes might spot it, but it won't change how our test performs. Keep up the good work!
app/client/cypress/fixtures/TestDataSet1.json (1)
444-457: Class, let's examine the changes in our test data.Now, children, I want you to pay close attention to the modifications in our
TableURLColumnTypesection. Can anyone tell me what's different? That's right, we've updated our image URLs! Instead of generic wallpapers, we now have Appsmith-specific images. This is a wonderful improvement that makes our test data more relevant to our application.Let's break it down:
- We've replaced the wallpaper images with Appsmith logos and screenshots.
- The new URLs point to the Appsmith documentation site.
- Each image is now directly related to Appsmith, which is the subject of our tests.
Can anyone tell me why this change is important? Yes, that's correct! It helps us create more realistic and application-specific test scenarios. Well done, class!
app/client/cypress/e2e/Regression/ClientSide/Widgets/DocumentViewer/DocumentViewer_spec.ts
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/Modal/Modal_spec.ts
Show resolved
Hide resolved
|
/ci-test-limit-count run_count=12 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11119069401. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11119069401. |
Please ignore this failure as changes reverted for this file |
sagar-qa007
added
ok-to-test
app/client/cypress/e2e/Regression/ClientSide/Widgets/DocumentViewer/DocumentViewer_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV1/TableBugs_Spec.ts
Outdated
Show resolved
Hide resolved
sagar-qa007
added
ok-to-test
| const jpgImg = "https://jpeg.org/images/jpegsystems-home.jpg"; | ||
| const gifImg = | ||
| "https://mir-s3-cdn-cf.behance.net/project_modules/max_1200/5eeea355389655.59822ff824b72.gif"; | ||
| const jpgImg = |
There was a problem hiding this comment.
@sagar-qa007 QQ: These images are still external to the Appsmith product. What would happen if the Devrel team decided to change or remove this image? Wouldn't this still lead to flakiness in the product?
Would it be possible to bring these images in control of TED? Or is there a technical limitation in doing so?
There was a problem hiding this comment.
We initially attempted to store the Docker images locally, but the test case requires loading an image via HTTPS for the deployed application. While adding the images to a CDN was considered, it was not deemed an ideal solution. However, the logos selected are from stable sources that are unlikely to change frequently, minimizing potential issues with third-party hosting within appsmith.
There was a problem hiding this comment.
@sagar-qa007 Why was a CDN solution deemed not ideal? Can you throw some light into this?
Description
Updated third party URLs.
Fixes
[Issue URL](https://github.com/appsmithorg/appsmith/issues/36437)Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11174864458
Commit: c6432d4
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 04 Oct 2024 07:58:04 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Chores