test: add e2e tests for tsc, slack and ambassadors page#4244
test: add e2e tests for tsc, slack and ambassadors page#4244asyncapi-bot merged 49 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds three new Cypress page objects (SlackPage, TSCPage, AmbassadorsPage), extends HomePage with two navigation helpers, and adds three test suites for Slack invites, TSC newsletter/social checks, and Ambassadors page verifications. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Cypress Test
participant Home as HomePage
participant TSC as TSCPage
Test->>Home: goToTSCPage()
Home-->>Test: navigated to /community/tsc
Test->>TSC: hoverCommunityLink()
Test->>TSC: fillNewsletterForm(name, email)
Test->>TSC: submitNewsletter()
TSC-->>Test: getSuccessMessage() / getFailureMessage()
Note right of TSC: Optional: verifyTSCMemberSocialLinks(name, links)
sequenceDiagram
participant Test as Cypress Test
participant Slack as SlackPage
Test->>Slack: visitSlack()
Slack-->>Test: waitForPageLoad()
alt invite active
Test->>Slack: verifyAllLoginMethods()
else invite inactive
Test->>Slack: verifyInactiveLinkMessage()
end
Test->>Slack: verifyAllFooterLinks()
sequenceDiagram
participant Test as Cypress Test
participant Home as HomePage
participant Amb as AmbassadorsPage
Test->>Home: goToAmbassadorsPage()
Home-->>Test: navigated to /community/ambassadors
Test->>Amb: verifyKeySectionsAndLinks()
Note right of Amb: iterate -> verifyAmbassadorSocialLinks(name, links)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4244 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 830 830
Branches 159 159
=========================================
Hits 830 830 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
cypress/pages/tscpage.js (1)
1-4: Clean up formatting and add proper class documentation.Consider adding a class comment describing the purpose of this page object and removing unnecessary blank lines.
- class TSCPage { - - + /** + * Page object for TSC (Technical Steering Committee) page interactions + */cypress/pages/slack.js (2)
3-5: Consider making the Slack URL configurable.The hardcoded URL could become outdated or may need to be different for different environments.
- visitSlack(){ - cy.visit('https://asyncapi.slack.com/join/shared_invite/zt-33bsaqqgz-ZL0a3ZUiuy4stSbXB~~E9A#/shared-invite/email'); - } + visitSlack(url = 'https://asyncapi.slack.com/join/shared_invite/zt-33bsaqqgz-ZL0a3ZUiuy4stSbXB~~E9A#/shared-invite/email'){ + cy.visit(url); + }
24-31: Improve consistency in assertion patterns.Consider using more specific assertions and consistent patterns across verification methods.
verifyPrivacyAndTerms() { cy.get('[href="/legal"]') - .should('have.attr', 'href', '/legal'); + .should('have.attr', 'href', '/legal') + .and('be.visible'); } verifyContactUs(){ cy.get('[href="/help/requests/new"]') - .should('have.attr', 'href', '/help/requests/new'); + .should('have.attr', 'href', '/help/requests/new') + .and('be.visible'); }cypress/tscpage.cy.js (1)
32-46: Improve formatting and structure of link verification test.The test has inconsistent spacing and formatting issues that affect readability.
- - it('verifies key links on the TSC page', () => { - - const linksToVerify = [ - {href: 'https://github.com/asyncapi/community/blob/master/TSC_MEMBERSHIP.md',label:'Link'}, - {href: 'https://github.com/asyncapi/community/blob/master/CHARTER.md',label:'Open Governance Model'}, - {href: 'https://www.asyncapi.com/blog/governance-motivation',label:'this'} - ]; - - linksToVerify.forEach(({ href,label }) => { - cy.get(`a[href="${href}"]`).contains(label); - - - }) - }); + it('verifies key links on the TSC page', () => { + const linksToVerify = [ + { href: 'https://github.com/asyncapi/community/blob/master/TSC_MEMBERSHIP.md', label: 'Link' }, + { href: 'https://github.com/asyncapi/community/blob/master/CHARTER.md', label: 'Open Governance Model' }, + { href: 'https://www.asyncapi.com/blog/governance-motivation', label: 'this' } + ]; + + linksToVerify.forEach(({ href, label }) => { + cy.get(`a[href="${href}"]`).contains(label).should('be.visible'); + }); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cypress/pages/homepage.js(2 hunks)cypress/pages/slack.js(1 hunks)cypress/pages/tscpage.js(1 hunks)cypress/slackworkspace.cy.js(1 hunks)cypress/tscpage.cy.js(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: asyncapi-bot
PR: asyncapi/website#0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Learnt from: sagarkori143
PR: asyncapi/website#0
File: :0-0
Timestamp: 2025-06-20T14:47:22.389Z
Learning: The AsyncAPI website modularization PR uses Promise.allSettled for parallel execution of build tasks, which provides better performance and fault isolation compared to sequential execution.
cypress/slackworkspace.cy.js (1)
Learnt from: vishvamsinh28
PR: asyncapi/website#3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
cypress/tscpage.cy.js (6)
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is enforced by the project's configuration which uses `moduleResolution: "bundler"` in tsconfig.json and TypeScript-aware ESLint plugins. The .ts extensions are required even though the files are imported using CommonJS require statements.
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Learnt from: akshatnema
PR: asyncapi/website#3101
File: tests/fixtures/rssData.js:1-57
Timestamp: 2024-11-01T13:32:15.472Z
Learning: In the `tests/fixtures/rssData.js` file of the `@asyncapi/website` project, tests for edge cases such as empty strings for title or excerpt, very long text content, international characters (UTF-8), or malformed URLs in `slug` or `cover` are not necessary because these cases will not occur.
Learnt from: vishvamsinh28
PR: asyncapi/website#3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In JavaScript test files within the AsyncAPI website project, TypeScript file imports must include the .ts extension to avoid lint errors, even though the files being imported are JavaScript files.
cypress/pages/tscpage.js (2)
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In JavaScript test files within the AsyncAPI website project, TypeScript file imports must include the .ts extension to avoid lint errors, even though the files being imported are JavaScript files.
⏰ 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: Lighthouse CI
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-13
🔇 Additional comments (4)
cypress/pages/tscpage.js (1)
5-29: LGTM! Well-structured page object methods.The page object methods are well-implemented with appropriate selectors and follow good Cypress patterns. The use of
data-testidattributes is excellent for test stability.cypress/slackworkspace.cy.js (1)
1-15: LGTM! Clean and well-structured test implementation.The test file follows good Cypress patterns with proper page object usage. The test case systematically verifies all the UI elements on the Slack workspace page.
cypress/pages/homepage.js (1)
1-2: LGTM! Proper imports added for page objects.The imports for ToolsPage and DocsPage are correctly added to support the new navigation methods.
cypress/tscpage.cy.js (1)
8-14: LGTM! Well-structured test setup.The beforeEach hook properly initializes page objects and navigates to the correct page for testing.
anshgoyalevil
added
the
gsoc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cypress/slackworkspace.cy.js (1)
1-21: Consider adding error handling and test isolation.While the current implementation is solid, consider these enhancements for robustness:
import SlackPage from './pages/slack'; describe('Slack workspace tests', () => { const slackPage = new SlackPage(); beforeEach(() => { slackPage.visitSlack(); }); + afterEach(() => { + // Clean up any test state if needed + }); it('User can access all login methods', () => { slackPage.verifyGoogleLoginButton(); slackPage.verifyAppleLoginButton(); slackPage.verifyContinueWithEmail(); }); it('User can view links for Privacy, Contact Us, and Region Change', () => { slackPage.verifyPrivacyAndTerms(); slackPage.verifyContactUs(); slackPage.verifyChangeRegion(); }); });Additionally, consider adding test tags for better organization:
-describe('Slack workspace tests', () => { +describe('Slack workspace tests', { tags: ['@slack', '@e2e'] }, () => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cypress/pages/tscpage.js(1 hunks)cypress/slackworkspace.cy.js(1 hunks)cypress/tscpage.cy.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cypress/pages/tscpage.js
- cypress/tscpage.cy.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: asyncapi-bot
PR: asyncapi/website#0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Learnt from: sagarkori143
PR: asyncapi/website#0
File: :0-0
Timestamp: 2025-06-20T14:47:22.389Z
Learning: The AsyncAPI website modularization PR uses Promise.allSettled for parallel execution of build tasks, which provides better performance and fault isolation compared to sequential execution.
cypress/slackworkspace.cy.js (3)
Learnt from: JeelRajodiya
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-03T08:14:02.138Z
Learning: The user (JeelRajodiya) has stated a preference to keep the test files in CommonJS rather than migrating them to TypeScript.
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
⏰ 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). (1)
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (4)
cypress/slackworkspace.cy.js (4)
3-8: Good test suite structure with proper setup.The test suite follows Cypress best practices with a clear describe block and beforeEach hook for consistent test setup.
10-14: Well-structured test for login methods.The test case covers all login methods systematically. The test name clearly describes what is being verified.
16-20: Comprehensive test for privacy and contact links.The test case covers important user-facing elements like privacy terms, contact information, and region settings.
1-1: SlackPage methods verified – no issues foundAll required methods (visitSlack, verifyGoogleLoginButton, verifyAppleLoginButton, verifyContinueWithEmail, verifyPrivacyAndTerms, verifyContactUs, verifyChangeRegion) are present in cypress/pages/slack.js. No further changes needed.
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4244--asyncapi-website.netlify.app/ |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
cypress/ambassadors.cy.js (1)
19-50: Hardcoded ambassador data may cause test brittleness.The test embeds specific ambassador names and social links that could become outdated if ambassadors are removed, renamed, or update their profiles. This creates a maintenance burden and potential for false negatives.
Consider these approaches to reduce brittleness:
- Test only that the social link elements exist and have valid href patterns, without verifying specific ambassadors
- Fetch ambassador data dynamically from the page and verify structure/format rather than specific values
- Document that this test data should be reviewed periodically
Example of a more resilient approach:
it('verifies social links structure for ambassadors', () => { // Verify that at least one ambassador has properly formatted social links cy.get('[data-testid="Ambassadors-members"]').first().within(() => { cy.get('a[href*="github.com"]').should('have.attr', 'href').and('match', /^https:\/\/www\.github\.com\//); cy.get('a[href*="twitter.com"]').should('have.attr', 'href').and('match', /^https:\/\/www\.twitter\.com\//); cy.get('a[href*="linkedin.com"]').should('have.attr', 'href').and('match', /^https:\/\/www\.linkedin\.com\//); }); });cypress/pages/ambassadors.js (2)
6-19: Brittle selectors using full GitHub URLs.The selectors use complete GitHub URLs (lines 7, 14, 16) which are fragile and will break if the repository structure changes (e.g., branch rename from
mastertomain, file reorganization, or URL updates).Consider more resilient selector strategies:
- Use partial URL matching with
[href*="..."]- Add
data-testidattributes to these links in the source code- Select by link text content instead of href
verifyKeySectionsAndLinks() { - cy.get('a[href="https://github.com/asyncapi/community/blob/master/docs/050-mentorship-program/ambassador-program/AMBASSADOR_PROGRAM.md"]') + cy.get('a[href*="/community/blob/"][href*="/AMBASSADOR_PROGRAM.md"]') .should('be.visible'); cy.get('[data-testid="Ambassadors-Iframe"]') .should('be.visible'); cy.get('[data-testid="Ambassadors-members-main"]') .should('be.visible'); - cy.get('a[href="https://github.com/asyncapi/community/blob/master/AMBASSADOR_ORGANIZATION.md#are-you-interested-in-becoming-an-official-asyncapi-ambassador"]') + cy.get('a[href*="/AMBASSADOR_ORGANIZATION.md#are-you-interested"]') .should('be.visible'); - cy.get('a[href="https://www.asyncapi.com/blog/asyncapi-ambassador-program"]') + cy.get('a[href*="/blog/asyncapi-ambassador-program"]') .should('be.visible'); }
21-30: Add error handling for ambassador not found.The method assumes the ambassador exists on the page. If the ambassador is removed or renamed, the test will fail with a cryptic timeout error rather than a clear message.
Add an explicit assertion after finding the ambassador:
verifyAmbassadorSocialLinks(name, links) { cy.contains('[data-testid="Ambassadors-members-details"]', name) + .should('exist') .closest('[data-testid="Ambassadors-members"]') .within(() => { if (links.twitter) cy.get(`a[href="${links.twitter}"]`).should('be.visible'); if (links.github) cy.get(`a[href="${links.github}"]`).should('be.visible'); if (links.linkedin) cy.get(`a[href="${links.linkedin}"]`).should('be.visible'); }); }cypress/tscpage.cy.js (2)
28-46: Brittle selectors using full GitHub/blog URLs.Similar to the ambassadors page, these selectors use complete URLs that will break if repository structure or blog URL paths change.
Use partial URL matching for more resilience:
const linksToVerify = [ { - href: 'https://github.com/asyncapi/community/blob/master/TSC_MEMBERSHIP.md', + href: '/community/blob/', + contains: 'TSC_MEMBERSHIP.md', label: 'Link', }, { - href: 'https://github.com/asyncapi/community/blob/master/CHARTER.md', + href: '/community/blob/', + contains: 'CHARTER.md', label: 'Open Governance Model', }, { - href: 'https://www.asyncapi.com/blog/governance-motivation', + href: '/blog/governance-motivation', label: 'this', }, ]; - linksToVerify.forEach(({ href, label }) => { - cy.get(`a[href="${href}"]`).contains(label).should('be.visible'); + linksToVerify.forEach(({ href, contains, label }) => { + cy.get(`a[href*="${href}"]${contains ? `[href*="${contains}"]` : ''}`).contains(label).should('be.visible'); });
48-80: Hardcoded TSC member data may cause test brittleness.Similar to the ambassadors test, this embeds specific member names and social links that could become outdated, creating maintenance overhead.
Consider the same alternatives mentioned for the ambassadors test:
- Test structural validity rather than specific member data
- Fetch member data dynamically and verify format
- Document the need for periodic review
Additionally, note the inconsistency in link property names: this test uses
GitHub,github,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cypress/ambassadors.cy.js(1 hunks)cypress/pages/ambassadors.js(1 hunks)cypress/pages/homepage.js(2 hunks)cypress/pages/slack.js(1 hunks)cypress/pages/tscpage.js(1 hunks)cypress/tscpage.cy.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/pages/tscpage.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-01T13:32:15.472Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3101
File: tests/fixtures/rssData.js:1-57
Timestamp: 2024-11-01T13:32:15.472Z
Learning: In the `tests/fixtures/rssData.js` file of the `asyncapi/website` project, tests for edge cases such as empty strings for title or excerpt, very long text content, international characters (UTF-8), or malformed URLs in `slug` or `cover` are not necessary because these cases will not occur.
Applied to files:
cypress/tscpage.cy.js
📚 Learning: 2025-01-19T04:51:41.255Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Applied to files:
cypress/tscpage.cy.js
🧬 Code graph analysis (1)
cypress/ambassadors.cy.js (1)
cypress/tscpage.cy.js (1)
homePage(4-4)
🔇 Additional comments (2)
cypress/pages/homepage.js (1)
26-44: Inconsistent navigation patterns across methods.The navigation methods use two different approaches:
goToToolsPage()andgoToDocsPage()use.click()navigation, whilegoToTSCPage()andgoToAmbassadorsPage()use directcy.visit(). While this may be intentional (if TSC/Ambassadors aren't in the navbar), it creates an inconsistent API.Verify whether TSC and Ambassadors pages can be reached via navbar navigation. If they are accessible through the navbar, consider updating the methods to use
.click()for consistency:goToTSCPage(){ cy.get('[data-testid="Navbar-main"]').contains('Community').click(); cy.contains('TSC').click(); // or appropriate selector return new TSCPage(); }If direct navigation is required (e.g., these pages aren't in the navbar), consider adding JSDoc comments explaining why
cy.visit()is used for these methods.cypress/tscpage.cy.js (1)
14-25: Newsletter subscription tests may be flaky due to external dependencies.These tests rely on actual form submission to a backend service, making them vulnerable to:
- Network issues
- Backend downtime
- Rate limiting
- Email validation changes
Consider whether these tests should:
- Stub/mock the network request to control the response
- Be moved to a separate integration test suite with appropriate retries
- Use Cypress intercept to verify the request payload without actually submitting
it('should succeed in subscribing to the newsletter', () => { cy.intercept('POST', '**/newsletter**', { statusCode: 200 }).as('newsletter'); tscpage.fillNewsletterForm('anushka', 'valid@example.com'); tscpage.submitNewsletter(); cy.wait('@newsletter'); tscpage.getSuccessMessage().should('be.visible'); });
cypress/pages/slack.js
Outdated
| visitSlack(){ | ||
| cy.visit('https://asyncapi.slack.com/join/shared_invite/zt-3clk6rmc0-Cujl2fChHYnHDUwFKRlQCw#/shared-invite/email'); | ||
| } |
There was a problem hiding this comment.
Hardcoded invite URL will expire, causing test failures.
The Slack shared invite URL contains a time-limited token (zt-3clk6rmc0-Cujl2fChHYnHDUwFKRlQCw) that will eventually expire, breaking the test.
Consider one of these approaches:
- Store the invite URL in an environment variable or config file that can be easily updated
- Generate a fresh invite token before tests run (if Slack API supports this)
- Document the token expiration and update process for maintainers
visitSlack(){
- cy.visit('https://asyncapi.slack.com/join/shared_invite/zt-3clk6rmc0-Cujl2fChHYnHDUwFKRlQCw#/shared-invite/email');
+ cy.visit(Cypress.env('SLACK_INVITE_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.
| visitSlack(){ | |
| cy.visit('https://asyncapi.slack.com/join/shared_invite/zt-3clk6rmc0-Cujl2fChHYnHDUwFKRlQCw#/shared-invite/email'); | |
| } | |
| visitSlack(){ | |
| cy.visit(Cypress.env('SLACK_INVITE_URL')); | |
| } |
🤖 Prompt for AI Agents
In cypress/pages/slack.js around lines 3 to 5 the Slack invite URL is hardcoded
with a time-limited token which will expire and break tests; replace the
hardcoded URL by reading it from configuration (CYPRESS_ENV, cypress.env.json or
a config file) or an environment variable, add fallback validation that the
variable exists and fail fast with a clear message, and update project docs to
show how to rotate/update the invite or add a script to generate a fresh invite
via Slack API if available.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cypress/ambassadors.cy.js (1)
19-50: Extract test data to a separate fixture file.The ambassador data (names and social links) should be moved to a separate JSON fixture file as previously requested by princerajpoot20. This will improve maintainability, reusability, and test readability.
Consider creating a file like
cypress/fixtures/ambassadors-data.jsonand importing it in the test.
Inconsistent indentation detected.
As noted by Copilot, this file uses tabs while other test files in the codebase use spaces. Please convert to spaces for consistency.
cypress/pages/tscpage.js (1)
2-4: Remove unusedhoverCommunityLinkmethod.This method is not used in any test files and should be removed to improve maintainability.
Based on previous review feedback from princerajpoot20 and Copilot.
🔎 Proposed fix
- hoverCommunityLink() { - cy.get('[data-testid="NavItem-Link"]').contains('Community').trigger('mouseover'); - } -
🧹 Nitpick comments (1)
cypress/pages/tscpage.js (1)
11-13: Consider using a more specific button selector.The
Button-maindata-testid is generic. If the TSC page adds more primary buttons in the future, this selector might match unintended elements. Consider using a more specific selector likeNewsletterSubscribe-submit-buttonif available.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cypress/ambassadors.cy.jscypress/pages/ambassadors.jscypress/pages/homepage.jscypress/pages/tscpage.jscypress/slackworkspace.cy.jscypress/tscpage.cy.js
🚧 Files skipped from review as they are similar to previous changes (4)
- cypress/pages/ambassadors.js
- cypress/slackworkspace.cy.js
- cypress/tscpage.cy.js
- cypress/pages/homepage.js
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
📚 Learning: 2025-01-19T04:51:41.255Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Applied to files:
cypress/ambassadors.cy.js
📚 Learning: 2025-06-10T06:27:25.419Z
Learnt from: aminoxix
Repo: asyncapi/website PR: 4127
File: components/layout/CommunityLayout.tsx:179-179
Timestamp: 2025-06-10T06:27:25.419Z
Learning: In AsyncAPI's community data structure, ambassadors and TSC members are guaranteed to have GitHub accounts. The `github` field is required in both `Ambassador` and `Tsc` interfaces, and the data processing in `addAdditionalUserInfo()` converts GitHub usernames to full URLs, ensuring safe string operations when extracting usernames from URLs.
Applied to files:
cypress/pages/tscpage.js
🧬 Code graph analysis (1)
cypress/ambassadors.cy.js (1)
cypress/tscpage.cy.js (1)
homePage(4-4)
⏰ 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). (2)
- GitHub Check: cypress-run
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (7)
cypress/ambassadors.cy.js (3)
1-5: LGTM: Clean imports and variable declarations.The page object imports and variable declarations follow standard Cypress patterns and are consistent with other test files in the codebase.
7-12: LGTM: Proper test setup.The beforeEach hook correctly initializes page objects and navigates to the Ambassadors page, ensuring a clean state for each test.
14-17: LGTM: Clean and focused test.The test properly delegates verification to the page object method, maintaining good separation of concerns.
cypress/pages/tscpage.js (4)
6-9: LGTM!The
fillNewsletterFormmethod correctly uses Cypress commands with appropriate data-testid selectors for form input.
15-19: LGTM!The
getSuccessMessagemethod correctly returns a Cypress element matching the expected success text.
21-23: LGTM!The
getFailureMessagemethod correctly returns a Cypress element matching the expected failure text.
24-32: LGTM!The
verifyTSCMemberSocialLinksmethod correctly:
- Locates the TSC member by name within the appropriate container
- Uses Cypress
within()for proper scoping- Conditionally verifies social links, making the test flexible
The method name already follows the naming convention suggested in previous reviews.
princerajpoot20
left a comment
princerajpoot20
left a comment
There was a problem hiding this comment.
@anushkaaaaaaaa Can you please have a look at the previous review comments? They are all still unaddressed.
| const ambassadors = [ | ||
| { | ||
| name: 'Quetzalli Writes', | ||
| links: { | ||
| github: 'https://www.github.com/quetzalliwrites', | ||
| twitter: 'https://www.twitter.com/QuetzalliWrites', | ||
| linkedin: 'https://www.linkedin.com/in/quetzalli-writes' |
There was a problem hiding this comment.
Can you please extract this data into a JSON file?
since all the previous review comments are regarding hardcoded information as well as storage/extraction from JSON Files, i am not going ahead by making any changes as previously discussed, thankyou |
Signed-off-by: Anushka Sharan <anushkasharan05@gmail.com>
Signed-off-by: Anushka Sharan <anushkasharan05@gmail.com>
Signed-off-by: Anushka Sharan <anushkasharan05@gmail.com>
Signed-off-by: Anushka Sharan <anushkasharan05@gmail.com>
|
@princerajpoot20 please review, i have made changes that you had asked for across all my PRs, thankyou. |
|
@anushkaaaaaaaa sure, i'll do it by the end of this week |
anshgoyalevil
left a comment
anshgoyalevil
left a comment
There was a problem hiding this comment.
Approving this PR. Please handle any required changes (if any) in follow up PRs.
princerajpoot20
left a comment
princerajpoot20
left a comment
There was a problem hiding this comment.
lgtm. great work @anushkaaaaaaaa
|
|
/rtm |
Description
Related issue(s)
Summary by CodeRabbit
New Features
Tests