feat: add e2e tests #25
Conversation
WalkthroughThe project has integrated Playwright for automated browser testing, with configurations tailored for CI/CD environments. Tests are set to run in parallel with retries and trace collection on failures. The Changes
TipsChat with CodeRabbit Bot (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
levino
force-pushed
the
feature/e2e-tests
branch
from
1ef89b1 to
efc495e
Compare
levino
force-pushed
the
feature/e2e-tests
branch
from
efc495e to
1ecda59
Compare
levino
force-pushed
the
feature/e2e-tests
branch
from
1ecda59 to
e809568
Compare
levino
force-pushed
the
feature/e2e-tests
branch
from
e809568 to
292b583
Compare
levino
force-pushed
the
feature/e2e-tests
branch
from
292b583 to
5feb168
Compare
levino
force-pushed
the
feature/e2e-tests
branch
from
5feb168 to
3202f2e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- apps/registry/package.json
- pnpm-lock.yaml
- turbo.json
Files selected for processing (4)
- .github/workflows/playwright.yml (1 hunks)
- apps/registry/.gitignore (1 hunks)
- apps/registry/playwright.config.js (1 hunks)
- apps/registry/tests/render.spec.js (1 hunks)
Files skipped from review due to trivial changes (1)
- apps/registry/.gitignore
Additional comments: 7
.github/workflows/playwright.yml (3)
- 17-17:
Verify ifpnpmis pre-installed on GitHub Actions runners. If it is, the commandnpm install -g pnpmis redundant and can be removed to save time and resources.- run: npm install -g pnpm && pnpm install + run: pnpm install
22-22:
Verify that the project uses Turbo and that thetest:e2escript is correctly defined in thepackage.jsonto run the Playwright tests.28-28:
Consider if a retention period of 30 days for test artifacts is necessary. If not, reducing this value can help save storage space on GitHub.apps/registry/playwright.config.js (3)
8-8:
Verify whether the environment variables should be loaded from a.envfile or if this is an intentional change.33-33:
Verify whether thebaseURLshould be set for the tests or if this is an intentional change.57-74:
Verify whether the mobile and branded browser tests are out of scope for this configuration or if they should be included.apps/registry/tests/render.spec.js (1)
- 8-8:
The use of a regular expression in the
toHaveTitleassertion is appropriate for flexibility in matching the title.
| // @ts-check | ||
| const { test, expect } = require('@playwright/test'); | ||
|
|
||
| test('has title', async ({ page }) => { | ||
| await page.goto('http://localhost:3002/thomasdavis'); | ||
|
|
||
| // Expect a title "to contain" a substring. | ||
| await expect(page).toHaveTitle(/Thomas Davis/); | ||
| }); |
There was a problem hiding this comment.
Consider using environment variables or a configuration file for the base URL (http://localhost:3002) to ensure the test is environment-agnostic.
| // @ts-check | ||
| const { test, expect } = require('@playwright/test'); | ||
|
|
||
| test('has title', async ({ page }) => { |
There was a problem hiding this comment.
The test name "has title" could be more descriptive, such as "Page title contains 'Thomas Davis'". This would provide more context when looking at test results.
levino
force-pushed
the
feature/e2e-tests
branch
from
3162fdf to
264466f
Compare
levino
force-pushed
the
feature/e2e-tests
branch
from
264466f to
89af6f7
Compare
|
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- apps/registry/package.json
- pnpm-lock.yaml
- turbo.json
Files selected for processing (5)
- .github/workflows/playwright.yml (1 hunks)
- apps/registry/.gitignore (1 hunks)
- apps/registry/pages/api/formatters/template.js (1 hunks)
- apps/registry/playwright.config.js (1 hunks)
- apps/registry/tests/render.spec.js (1 hunks)
Files skipped from review due to trivial changes (2)
- apps/registry/.gitignore
- apps/registry/pages/api/formatters/template.js
Additional comments: 10
.github/workflows/playwright.yml (1)
- 24-24:
Verify the necessity and permissions of the customLEVINS_TOKENand consider using the defaultGITHUB_TOKENprovided by GitHub Actions if it suffices for the workflow's requirements.
[SECURITY]apps/registry/playwright.config.js (8)
8-8:
The dotenv package is commented out, which may prevent environment variables from being loaded. Verify if this is intentional or an oversight.17-17:
ThereuseExistingServerproperty is correctly configured to depend on theCIenvironment variable, ensuring that existing servers are reused in local development but not in CI environments.23-23:
TheforbidOnlyproperty is correctly configured to prevent accidental commits of.onlyin test suites when running in CI environments.25-25:
Theretriesproperty is correctly configured to allow retries in CI environments, which can help mitigate flakiness in end-to-end tests.27-27:
Theworkersproperty is set to 1 in CI environments, which contradicts the summary's mention of full parallel test execution. Clarify whether this is intentional or a misconfiguration.29-29:
Thereporterproperty is set to 'html', which aligns with the summary and provides a visual report of test results.36-36:
Thetraceproperty is set to 'on-first-retry', which is a useful setting for capturing additional debugging information on test retries.40-54:
The projects for Chromium, Firefox, and WebKit are correctly configured with device-specific settings, ensuring tests are run on major browsers as intended.apps/registry/tests/render.spec.js (1)
- 4-9:
The test case name "has title" could be more descriptive, such as "Page title contains 'Thomas Davis'". This would provide more context when looking at test results. Additionally, consider using environment variables or a configuration file for the base URL (
http://localhost:3002) to ensure the test is environment-agnostic. Lastly, adding comments or documentation explaining the purpose of the test or the significance of the URL could improve maintainability.
.github/workflows/playwright.yml
Outdated
| with: | ||
| node-version: 18 | ||
| - name: Install dependencies | ||
| run: npm install -g pnpm && pnpm install |
There was a problem hiding this comment.
Consider using a more direct method to install pnpm that does not involve modifying the global npm installation, if possible.
There was a problem hiding this comment.
Instead of complaining, you could rather just propose a change to fix this problem...
There was a problem hiding this comment.
@levino My apologies for any confusion. Here's a proposed change to the workflow file that uses corepack to enable pnpm without global installation:
@@ -0,0 +1,30 @@
name: Playwright Tests
on:
push:
branches: [master]
pull_request:
branches: [master]
jobs:
test:
timeout-minutes: 60
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
node-version: 18
+ corepack: true
- name: Install dependencies
- run: npm install -g pnpm && pnpm install
+ run: corepack enable && pnpm installThis change enables corepack which is a tool that comes with Node.js to manage package managers like pnpm, yarn, etc., and allows you to use pnpm without installing it globally.
levino
force-pushed
the
feature/e2e-tests
branch
from
89af6f7 to
c5f6bca
Compare
levino
force-pushed
the
feature/e2e-tests
branch
3 times, most recently
from
9557bbc to
292e8df
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- apps/registry/package.json
- pnpm-lock.yaml
- turbo.json
Files selected for processing (6)
- .github/workflows/ci.yml (2 hunks)
- .github/workflows/playwright.yml (1 hunks)
- apps/registry/.gitignore (1 hunks)
- apps/registry/pages/api/[payload].js (2 hunks)
- apps/registry/playwright.config.js (1 hunks)
- apps/registry/tests/render.spec.js (1 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/playwright.yml
- apps/registry/.gitignore
Additional comments: 13
.github/workflows/ci.yml (3)
23-28:
The lint job setup with Node.js version 18 and pnpm package manager is correctly configured.38-48:
The build job setup with Node.js version 18, pnpm package manager, and Ruby with bundler cache is correctly configured.50-75:
The test job setup with Node.js version 18, pnpm package manager, and Playwright browsers installation is correctly configured. However, ensure that theLEVINS_TOKENused in the environment variableGITHUB_TOKENis intended for this purpose and has the necessary permissions.apps/registry/pages/api/[payload].js (2)
15-19:
The addition of the environment variable check forGITHUB_TOKENis a good security practice and the error message provided is clear for developers.81-87:
The conditional inclusion of the Authorization header in the HTTP request is a secure and flexible approach that allows for development without the token.apps/registry/playwright.config.js (7)
4-8:
Verify whether the dotenv package is intentionally commented out and ensure that environment variables are being loaded correctly in all required environments.17-17:
ThereuseExistingServerconfiguration is correctly set to depend on the 'CI' environment variable, which is a good practice for CI environments.21-21:
Ensure that test cases are designed to run in parallel without causing race conditions or shared state issues.25-25:
The retry configuration is set appropriately for CI environments to help mitigate test flakiness.27-27:
The worker configuration is set to 1 on CI to ensure reliable test execution, which is a good practice.33-33:
Verify whether thebaseURLis intentionally commented out and ensure that the correct base URL is being used in all required environments.39-75:
The configuration for Chromium, Firefox, and WebKit browsers is set up correctly for cross-browser testing. The commented-out configurations for mobile and branded browsers are good placeholders for future test scenarios.apps/registry/tests/render.spec.js (1)
- 4-9:
The previous comments regarding the hardcoded URL and the non-descriptive test name are still valid and applicable.
There was a problem hiding this comment.
I will go ahead and merge this in order to be able to merge #22. Please still read my comments @thomasdavis
| @@ -14,6 +14,10 @@ const { Client } = require('pg'); | |||
|
|
|||
| const GITHUB_TOKEN = process.env.GITHUB_TOKEN; | |||
|
|
|||
| if (!GITHUB_TOKEN && process.env.NODE_ENV !== 'development') { | |||
| throw new Error('GITHUB_TOKEN is not set in environment variables'); | |||
There was a problem hiding this comment.
When I did not specify the GITHUB_TOKEN in the environment, I would get an error message, that the github username does not exist, which made it harder to debug. This route will get issues, if we do not set a GITHUB_TOKEN because, as far as I understand, we will hit rate limits with the github api. This is why I think it is a good idea to fail hard and gracefully here instead of failing weirdly later
| @@ -77,7 +81,7 @@ export default async function handler(req, res) { | |||
| `https://api.github.com/users/${username}/gists?per_page=100`, | |||
| { | |||
| headers: { | |||
| Authorization: 'Bearer ' + GITHUB_TOKEN, | |||
| ...(GITHUB_TOKEN ? { Authorization: 'Bearer ' + GITHUB_TOKEN } : {}), // If we have no token and are in development, we can still make some requests. | |||
There was a problem hiding this comment.
This is a somewhat complicated but I find neat way to not set any headers when we have no GITHUB_TOKEN. The request will go through with any headers but will not with Authorization: NULL header. For development purposes (see also check above) it works without the GITHUB_TOKEN just fine. If someone hits rate limits during development, they can still set the token if they want.
levino
force-pushed
the
feature/e2e-tests
branch
from
292e8df to
d511f0f
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- apps/registry/package.json
- pnpm-lock.yaml
- turbo.json
Files selected for processing (5)
- .github/workflows/ci.yml (2 hunks)
- apps/registry/.gitignore (1 hunks)
- apps/registry/pages/api/[payload].js (2 hunks)
- apps/registry/playwright.config.js (1 hunks)
- apps/registry/tests/render.spec.js (1 hunks)
Files skipped from review due to trivial changes (1)
- apps/registry/.gitignore
Additional comments: 12
.github/workflows/ci.yml (3)
23-28:
The lint job setup with Node.js and pnpm is correctly configured.38-48:
The build job setup with Node.js, pnpm, and Ruby is correctly configured.50-74:
The test job setup with Playwright is correctly configured, including the installation of browsers and running of tests.apps/registry/pages/api/[payload].js (2)
15-19:
The addition of the environment variable check forGITHUB_TOKENis a good practice to prevent obscure errors and enforce proper configuration in non-development environments.81-87:
The implementation of the conditional Authorization header based on theGITHUB_TOKENallows for easier development without the token and is a good practice for maintaining flexibility.apps/registry/playwright.config.js (7)
4-8:
Verify whether the environment variables required for the Playwright configuration are being set correctly in all environments, given thatdotenvis commented out. Ifdotenvis necessary for local development or other non-CI environments, consider uncommenting it or providing clear instructions on how to set the required environment variables.17-17:
The use ofreuseExistingServeris correctly configured to improve efficiency during local development by reusing the server when not running in a CI environment.23-23:
TheforbidOnlyconfiguration is correctly set to prevent accidentally committing.onlyin test cases, which could lead to incomplete test runs in CI.25-25:
Theretriesconfiguration is set appropriately to handle potential flakiness in CI environments by allowing tests to be retried up to 2 times.29-29:
Ensure that the CI system can handle and display HTML reports, or that there is a process in place to review these reports after test runs, as thereporteris set to 'html'.35-36:
Thetraceconfiguration is set to 'on-first-retry', which is a good practice for debugging flaky tests by providing a trace of the first retry attempt.40-54:
The configuration of projects for Chromium, Firefox, and WebKit browsers is a good practice for ensuring cross-browser compatibility of the application.
| test('has title', async ({ page }) => { | ||
| await page.goto('http://localhost:3002/thomasdavis'); | ||
|
|
||
| // Expect a title "to contain" a substring. | ||
| await expect(page).toHaveTitle(/Thomas Davis/); | ||
| }); |
There was a problem hiding this comment.
Consider using environment variables or a configuration file for the base URL (http://localhost:3002) to ensure the test is environment-agnostic. Additionally, the test name "has title" could be more descriptive, such as "Page title contains 'Thomas Davis'". This would provide more context when looking at test results. It would also be beneficial to include comments or documentation explaining the purpose of the test or the significance of the URL for maintainability.
| /* Retry on CI only */ | ||
| retries: process.env['CI'] ? 2 : 0, | ||
| /* Opt out of parallel tests on CI. */ | ||
| workers: process.env['CI'] ? 1 : undefined, |
There was a problem hiding this comment.
Consider the impact of setting workers to 1 in CI environments, as this will cause tests to run sequentially and could lead to longer test execution times. If parallel execution is stable in the CI environment, allowing more workers could speed up the test suite.
Summary by CodeRabbit
New Features
Documentation
.gitignoreto exclude test results and reports from version control.Bug Fixes
Chores
THEMESobject.Tests