test: add e2e tests for tsc, slack and ambassadors page #4244
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 Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 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
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-01-19T04:51:41.255ZApplied to 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). (5)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting 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 |
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 796 796
Branches 146 146
=========================================
Hits 796 796 ☔ 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="/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="/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.
Pull request overview
This PR adds comprehensive end-to-end (e2e) tests for three AsyncAPI community pages: TSC (Technical Steering Committee), Slack workspace, and Ambassadors. The implementation follows the established page object pattern used throughout the Cypress test suite, with separate test spec files and corresponding page object classes.
Key Changes:
- Added three new test suites covering newsletter subscription, login methods, footer navigation, and social link verification
- Implemented page object classes (TSCPage, SlackPage, AmbassadorsPage) with reusable helper methods
- Extended HomePage with direct navigation methods for TSC and Ambassadors pages
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| cypress/tscpage.cy.js | New test suite for TSC page testing newsletter subscription (success/failure) and member social links verification |
| cypress/slackworkspace.cy.js | New test suite for Slack workspace testing login methods, inactive invite handling, and footer links |
| cypress/ambassadors.cy.js | New test suite for Ambassadors page testing key sections and ambassador social links |
| cypress/pages/tscpage.js | Page object for TSC page with methods for newsletter form interaction and member link verification |
| cypress/pages/slack.js | Page object for Slack page with methods for login verification and link status checking |
| cypress/pages/ambassadors.js | Page object for Ambassadors page with methods for section/link verification |
| cypress/pages/homepage.js | Added goToTSCPage() and goToAmbassadorsPage() navigation methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hey @anushkaaaaaaaa, I will review this the day after tomorrow. |
princerajpoot20
left a comment
princerajpoot20
left a comment
There was a problem hiding this comment.
@anushkaaaaaaaa Please have a look at the review comments.
cypress/ambassadors.cy.js
Outdated
| 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' | ||
| } | ||
| }, | ||
| { | ||
| name: 'Daniel Kocot', | ||
| links: { | ||
| github: 'https://www.github.com/danielkocot', | ||
| twitter: 'https://www.twitter.com/dk_1977', | ||
| linkedin: 'https://www.linkedin.com/in/danielkocot' | ||
| } | ||
| }, | ||
| { | ||
| name: 'Hugo Guerrero', | ||
| links: { | ||
| github: 'https://www.github.com/hguerrero', | ||
| twitter: 'https://www.twitter.com/hguerreroo', | ||
| linkedin: 'https://www.linkedin.com/in/hugoguerrero' | ||
| } | ||
| } | ||
| ]; |
There was a problem hiding this comment.
Could you please extract all these constant data? This should not be part of the test.
Keeping it in a separate JSON data file makes it reusable, easier to locate and update when links change, and also makes the test case more readable. Keeping it here makes the test case unnecessarily bulky and less readable.
Decide the location of the JSON file accordingly, depending on the intended scope of the file and where it should be reusable.
| } | ||
|
|
||
| verifyKeySectionsAndLinks() { | ||
| cy.get('a[href="https://github.com/asyncapi/community/blob/master/docs/020-governance-and-policies/AMBASSADOR_PROGRAM.md"]' |
There was a problem hiding this comment.
The same goes for this as well—please create a separate JSON file that contains all the Markdown files. As you may be aware, we recently went through a major restructuring of the docs in the community repo, and we are still updating links on the website even now. The reason is the same: everywhere we needed those links, they were hardcoded.
Instead, we should have a single source of truth, which will make it much easier to update things in the future.
| class SlackPage { | ||
| visitSlack() { | ||
| cy.visit( | ||
| 'https://asyncapi.slack.com/join/shared_invite/zt-3clk6rmc0-Cujl2fChHYnHDUwFKRlQCw#/shared-invite/email', |
There was a problem hiding this comment.
Umm, if I’m not wrong, this is the Slack invite link. Don’t you think this link keeps changing? Could you replace it with the current source we have, so we can maintain a single source of truth here?
| { | ||
| 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'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
As I mentioned before, please extract these out.
| { | ||
| name: 'Aishat Muibudeen', | ||
| links: { | ||
| GitHub: 'https://www.github.com/Mayaleeeee', | ||
| Twitter: 'https://www.twitter.com/maya_ux_ui', | ||
| Linkedin: 'https://www.linkedin.com/in/aishatmuibudeen' | ||
| } | ||
| }, | ||
| { | ||
| name: 'Khuda Dad Nomani', | ||
| links: { | ||
| GitHub: 'https://www.github.com/KhudaDad414', | ||
| Twitter: 'https://www.twitter.com/KhudaDadNomani', | ||
| Linkedin: 'https://www.linkedin.com/in/khudadadnomani' | ||
| } | ||
| }, | ||
| { | ||
| name: 'Lukasz Gornicki', | ||
| links: { | ||
| GitHub: 'https://www.github.com/derberg', | ||
| Twitter: 'https://www.twitter.com/derberq', | ||
| Linkedin: 'https://www.linkedin.com/in/lukasz-gornicki-a621914' | ||
| } | ||
| } | ||
| ]; |
There was a problem hiding this comment.
Hmm… as mentioned above, please move this hardcoded data out of the test file.
cypress/tscpage.cy.js
Outdated
| ]; | ||
|
|
||
| members.forEach(({ name, links }) => { | ||
| tscpage.verifyMemberSocialLinks(name, links); |
There was a problem hiding this comment.
Please keep your naming according to clean coding practices.
Right now, it’s confusing and unclear. What do you mean by member here? It’s TscMember.
Please cross-check all the naming conventions you have used, as it’s very important to follow clean coding practices in order to keep the code maintainable.
|
FYI: #4533 (review) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…d Slack workspace pages.
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?
5 participants
Description
Related issue(s)
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.