test: add e2e tests for pages under tools dropdown#4533
test: add e2e tests for pages under tools dropdown#4533princerajpoot20 merged 42 commits intoasyncapi:masterfrom
Conversation
anushkaaaaaaaa
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughIntroduces a centralized Cypress BasePage with common helpers, updates many page objects to extend it, standardizes visit/link helpers, adds fixtures for tools, adds several new tools page objects and new E2E tests, and consolidates link verification into a generalized method. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
anushkaaaaaaaa
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4533 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 796 796
Branches 146 146
=========================================
Hits 796 796 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4533--asyncapi-website.netlify.app/ |
![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
🧹 Nitpick comments (2)
cypress/pages/toolsGenerator.js (1)
6-8: Extract duplicated helper to reduce code duplication.The
verifyHeadingExistsmethod is duplicated identically intoolsModelina.js(lines 6-8) andtoolsMisc.js(lines 14-16). Consider extracting this to a base page class or a shared helper module.Example approach - create a base page class:
// cypress/pages/basePage.js class BasePage { verifyHeadingExists(text) { cy.contains('h1, h2, h3, h4, h5, h6', text).should('be.visible'); } } export default BasePage;Then extend it:
+import BasePage from './basePage'; + -class ToolsGeneratorPage { +class ToolsGeneratorPage extends BasePage { visit() { cy.visit('/tools/generator'); } - verifyHeadingExists(text) { - cy.contains('h1, h2, h3, h4, h5, h6', text).should('be.visible'); - } - verifyHeader() { this.verifyHeadingExists('Docs, Code, Anything!'); }cypress/pages/toolsModelina.js (1)
26-28: Consider using 'be.visible' for stronger assertion.The assertion uses
.should('exist')which only checks DOM presence. For e2e tests,.should('be.visible')provides stronger verification that users can actually see the install snippet.Apply this change (and similarly in
toolsMisc.jslines 38, 53):verifyInstallSnippet() { - cy.contains('code', 'npm install @asyncapi/modelina').should('exist'); + cy.contains('code', 'npm install @asyncapi/modelina').should('be.visible'); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cypress/pages/toolsGenerator.js(1 hunks)cypress/pages/toolsMisc.js(1 hunks)cypress/pages/toolsModelina.js(1 hunks)cypress/tools_generator.cy.js(1 hunks)cypress/tools_misc.cy.js(1 hunks)cypress/tools_modelina.cy.js(1 hunks)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (6)
cypress/tools_generator.cy.js (2)
1-8: LGTM! Clean test setup.The test file follows Cypress best practices with proper page object instantiation and beforeEach hook for navigation.
10-24: LGTM! Well-structured test cases.The test cases are focused and follow the single-responsibility principle, making them easy to maintain and debug.
cypress/tools_modelina.cy.js (1)
1-25: LGTM! Consistent test structure.This test file follows the same pattern as the other tools tests, maintaining consistency across the test suite.
cypress/tools_misc.cy.js (1)
1-56: LGTM! Well-organized test suite.The use of nested describe blocks effectively organizes tests for multiple related pages, making the test suite easy to navigate and maintain.
cypress/pages/toolsMisc.js (1)
1-72: LGTM! Well-organized multi-page page object.The clear separation of methods with comments for each page (CLI, Parsers, GitHub Actions) makes this page object easy to navigate and maintain.
cypress/pages/toolsGenerator.js (1)
18-22: The review comment is based on incorrect assumptions and should be disregarded.The tests are actually correct in expecting
https://www.github.com/URLs because the source code inpages/tools/generator.tsx(line 25) and related tool pages explicitly use this format. The tests are properly validating what the page actually renders.While www.github.com redirects to github.com (the canonical form), both URLs are functional. If using canonical GitHub URLs is a goal, the fix should be in the source code files (
pages/tools/*.tsx), not in the test expectations (cypress/pages/*.js). The tests are doing their job correctly—they verify the rendered output matches the href attributes in the actual source components.Likely an incorrect or invalid review comment.
princerajpoot20
left a comment
princerajpoot20
left a comment
There was a problem hiding this comment.
@anushkaaaaaaaa
Please have a look at the review comments.
I stopped reviewing the PR in between, for the reason I mentioned in the review above: there are major changes that need to be done across all the open PRs you have so far (#4551, #4548, #4533, #4244, #4215).
Hence, I thought there was no point in continuing the review as there are major changes required. Ping me once you are done with all the changes in all those open PRs. I will then give another review.
Also, I am not sure if you have the same pattern in the previous PRs that you have already merged. But please review all your work, regardless of whether it is merged. We can still fix it easily. It will be difficult at a later stage. We should not have so much repetition in functions. This violates the DRY principle and will make the code very difficult to maintain in the long run. Adding a simple test will require a lot of repetitive work, and we don’t want to reach that state.
Thanks.
cypress/pages/toolsGenerator.js
Outdated
| @@ -0,0 +1,31 @@ | |||
| class ToolsGeneratorPage { | |||
There was a problem hiding this comment.
Nit: consider naming class consistently with file name. Reivew these naming in all the files
cypress/pages/toolsGenerator.js
Outdated
| cy.visit('/tools/generator'); | ||
| } | ||
|
|
||
| verifyHeadingExists(text) { |
There was a problem hiding this comment.
VerifyHeadingExists is duplicated across multiple pages in this PR and also in the all the other PRs you have raised.
Consider extracting it into a shared base helper to keep things DRY and easier to maintain.
There was a problem hiding this comment.
@princerajpoot20 can you please elaborate this more
I didn't get it
There was a problem hiding this comment.
@sambhavgupta0705 This verifyHeadingExists function is duplicated in every file. It repeats the same logic again and again across multiple files and multiple PRs of @anushkaaaaaaaa. We need to remove this duplication and keep the code DRY.
cypress/pages/toolsGenerator.js
Outdated
| verifyMainImage() { | ||
| cy.get('img[alt="generator diagram"]').should('be.visible'); | ||
| } |
There was a problem hiding this comment.
consider asserting more strongly like src is non-empty. also please review the naming of function. What do you mean by mainImage?
cypress/pages/toolsMisc.js
Outdated
| } | ||
|
|
||
| verifyHeadingExists(text) { | ||
| cy.contains('h1, h2, h3, h4, h5, h6', text).should('be.visible'); |
There was a problem hiding this comment.
as mentioned earlier. please follow DRY principle
cypress/pages/toolsMisc.js
Outdated
| cy.visit('/tools/cli'); | ||
| } | ||
|
|
||
| visitParsers() { | ||
| cy.visit('/tools/parsers'); | ||
| } | ||
|
|
||
| visitGithubActions() { | ||
| cy.visit('/tools/github-actions'); |
There was a problem hiding this comment.
Please review these dedicated visit() methods that are everywhere. They are repeating the same stuff everywhere. Consider using a single helper like visit(path). This reduces repetition and makes your code DRY.
Please review all the methods in all the tests you have added via multiple PRs you have opened right now. If they can be extracted out into shared functions, it will make them reusable and keep the code DRY.
cypress/pages/toolsMisc.js
Outdated
| verifyCliHeader() { | ||
| this.verifyHeadingExists( | ||
| 'Interact with AsyncAPI from the comfort of your CLI', | ||
| ); | ||
| } | ||
|
|
||
| verifyCliGithubLink() { | ||
| cy.contains('a', 'View on Github') | ||
| .should('be.visible') | ||
| .and('have.attr', 'href', 'https://www.github.com/asyncapi/cli'); | ||
| } | ||
|
|
||
| verifyCliDocsLink() { | ||
| cy.contains('a', 'View Docs') | ||
| .should('be.visible') | ||
| .and('have.attr', 'href', '/docs/tools/cli'); | ||
| } |
There was a problem hiding this comment.
@anushkaaaaaaaa
As mentioned earlier,
all these can be removed. This is unnecessary repetition of code.
Make this a general function that takes parameters and move it to a shared folder so it can be used by everyone and you don’t have to write the same code again and again. Please refer to the DRY principle. This will make the code maintainable.
I have observed this across all your PRs that are open right now, so please review all your PRs for this.
cypress/pages/toolsMisc.js
Outdated
| verifyParsersGithubLink() { | ||
| cy.contains('a', 'View on Github') | ||
| .should('be.visible') | ||
| .and('have.attr', 'href', 'https://www.github.com/asyncapi/parser-js'); | ||
| } | ||
|
|
||
| verifyParsersInstallSnippet() { | ||
| cy.contains('code', 'npm install @asyncapi/parser').should('exist'); | ||
| } | ||
|
|
||
| // GitHub Actions page checks | ||
| verifyGhActionsHeader() { | ||
| this.verifyHeadingExists('Automate using GitHub Actions'); | ||
| } | ||
|
|
||
| verifyGhActionsGithubLink() { | ||
| cy.contains('a', 'View on Github') | ||
| .should('be.visible') | ||
| .and( | ||
| 'have.attr', | ||
| 'href', | ||
| 'https://www.github.com/asyncapi/github-action-for-generator', | ||
| ); | ||
| } |
There was a problem hiding this comment.
As mentioned above, you should avoid these and make a general function that takes parameters
cypress/pages/toolsMisc.js
Outdated
| } | ||
| } | ||
|
|
||
| export default ToolsMiscPage; |
There was a problem hiding this comment.
Please review this naming
cypress/pages/toolsModelina.js
Outdated
| visit() { | ||
| cy.visit('/tools/modelina'); | ||
| } | ||
|
|
||
| verifyHeadingExists(text) { | ||
| cy.contains('h1, h2, h3, h4, h5, h6', text).should('be.visible'); | ||
| } | ||
|
|
||
| verifyHeader() { | ||
| this.verifyHeadingExists('Modelina'); | ||
| } | ||
|
|
||
| verifyGithubLink() { | ||
| cy.contains('a', 'View on Github') | ||
| .should('be.visible') | ||
| .and('have.attr', 'href', 'https://www.github.com/asyncapi/modelina'); | ||
| } | ||
|
|
||
| verifyTryItNowLink() { | ||
| cy.contains('a', 'Try it now') | ||
| .should('be.visible') | ||
| .and('have.attr', 'href', 'https://modelina.org/playground'); | ||
| } | ||
|
|
||
| verifyInstallSnippet() { |
There was a problem hiding this comment.
I hope you are now able to understand the real benefit. If you had made general methods, you wouldn’t have to put repetitive effort into doing the same stuff. And most importantly, it makes the code maintainable.
…CLI, Parsers, GitHub Actions, and Modelina.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cypress/tools_modelina.cy.js`:
- Around line 15-21: Update the two test case description strings to use
consistent capitalization and fix the typo: change the first it(...) title from
'verifying install snippet' to 'Verifying install snippet' and change the second
it(...) title from 'Verifiying the GitHub button link' to 'Verifying the GitHub
button link'; these are the human-readable names passed to the it(...) blocks
that wrap calls to page.verifyElementContainsText and page.verifyButtonLink,
respectively.
🧹 Nitpick comments (3)
cypress/pages/Footer.js (1)
10-12: Assert a single visible footer instead of silently picking the first.
Selecting.first()can mask regressions where multiple visible footers appear. Consider asserting exactly one visible footer (or use a more specific selector/data-testid).♻️ Suggested tweak
- cy.get(this.footerSelector).filter(':visible').first().within(() => { + cy.get(this.footerSelector) + .filter(':visible') + .should('have.length', 1) + .first() + .within(() => { this.verifyLink(href, text, attr); });cypress/tools_generator.cy.js (1)
1-5: Inconsistent class naming convention.The import and instantiation use camelCase (
toolsGenerator), while other similar test files in this PR use PascalCase (ToolsMisc,ToolsModelina). For consistency across the test suite and adherence to JavaScript class naming conventions, use PascalCase.♻️ Suggested fix
-import toolsGenerator from './pages/toolsGenerator'; +import ToolsGenerator from './pages/toolsGenerator'; import toolsData from './fixtures/toolsPages.json'; describe('Tools - Generator Page', () => { - const page = new toolsGenerator(); + const page = new ToolsGenerator();cypress/pages/BaseHeaderPage.js (1)
3-5: Note:visit()no longer returnsthisfor fluent chaining.The previous implementation likely had
return this;to allow method chaining. If any tests rely on chaining aftervisit(), this change would break them. For header-related tests that always start from the homepage, the hardcoded'/'path is likely intentional.♻️ Consider preserving fluent chaining if needed
visit() { - cy.visit('/') + cy.visit('/'); + return this; }
…mplementing a page object model with a new BasePage.
…into toolsPage
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Please add some reasoning before closing PRs directly. You closed all five PRs that were under review and created similar new PRs instead. @anushkaaaaaaaa Could you please explain the reason for doing this? I’m not sure what benefit you expect from this approach. Whatever changes you want to make could have been done in the same branch. Keeping updates in the Please understand that reviewing PRs takes a significant amount of time. This simply makes the review process more time-consuming. FYR: Closed unmerged PRs: #4551, #4215, #4344, #4548, #4533 |
|
@anushkaaaaaaaa Please have a look at the AI code review. There are multiple minor nits. Overall, it looks good. Great work @anushkaaaaaaaa |
hello, i've made the changes you asked for, please review |
princerajpoot20
left a comment
princerajpoot20
left a comment
There was a problem hiding this comment.
LGTM!! Great work @anushkaaaaaaaa
|
princerajpoot20
added
the
gsoc
4 participants
Description
Related issue(s)
Summary by CodeRabbit
Tests
Refactor
Chores