Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

build: Add codecov test analytics for playwright tests #13654

Merged
merged 4 commits into from
Sep 13, 2024
Merged

build: Add codecov test analytics for playwright tests #13654

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b98bc08
build: Add codecov test analytics for playwright tests
AbhiPrasad Sep 10, 2024
2a010d1
Make sure reporter is not being overriden
AbhiPrasad Sep 10, 2024
c7b39f2
use easier to read condition
AbhiPrasad Sep 10, 2024
b4228b8
fix condition
AbhiPrasad Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,13 @@ jobs:
overwrite: true
retention-days: 7

- name: Upload test results to Codecov
if: cancelled() == 'false'
uses: codecov/test-results-action@v1
with:
directory: dev-packages/browser-integration-tests
token: ${{ secrets.CODECOV_TOKEN }}
Copy link

@codecov codecov bot Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code creates a potential problem where if the Codecov token is not set, the action may fail without a clear error. Consider adding error handling to handle the situation when secrets.CODECOV_TOKEN is not available.


job_browser_loader_tests:
name: PW ${{ matrix.bundle }} Tests
needs: [job_get_metadata, job_build]
Expand Down Expand Up @@ -653,6 +660,7 @@ jobs:
run: |
cd dev-packages/browser-integration-tests
yarn test:loader

- name: Upload Playwright Traces
uses: actions/upload-artifact@v4
if: failure()
Expand All @@ -662,6 +670,13 @@ jobs:
overwrite: true
retention-days: 7

- name: Upload test results to Codecov
if: cancelled() == 'false'
uses: codecov/test-results-action@v1
with:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeating the same actions in each workflow can be both inefficient and error-prone. Consider creating a reusable workflow to maintain this action in a single place.

directory: dev-packages/browser-integration-tests
token: ${{ secrets.CODECOV_TOKEN }}

job_check_for_faulty_dts:
name: Check for faulty .d.ts files
needs: [job_get_metadata, job_build]
Expand Down Expand Up @@ -1013,6 +1028,13 @@ jobs:
overwrite: true
retention-days: 7

- name: Upload test results to Codecov
if: cancelled() == 'false'
uses: codecov/test-results-action@v1
with:
directory: dev-packages/e2e-tests
token: ${{ secrets.CODECOV_TOKEN }}

job_optional_e2e_tests:
name: E2E ${{ matrix.label || matrix.test-application }} Test
# We only run E2E tests for non-fork PRs because the E2E tests require secrets to work and they can't be accessed from forks
Expand Down
2 changes: 1 addition & 1 deletion dev-packages/browser-integration-tests/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"test:loader:replay_buffer": "PW_BUNDLE=loader_replay_buffer yarn test:loader",
"test:loader:full": "PW_BUNDLE=loader_tracing_replay yarn test:loader",
"test:loader:debug": "PW_BUNDLE=loader_debug yarn test:loader",
"test:ci": "yarn test:all --reporter='line'",
"test:ci": "yarn test:all",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from specifying the reporter in the test script to be handled by the test configuration file has been noted. Please ensure the 'line' reporter is the intended default for local testing.

"test:update-snapshots": "yarn test:all --update-snapshots",
"test:detect-flaky": "ts-node scripts/detectFlakyTests.ts"
},
Expand Down
2 changes: 2 additions & 0 deletions dev-packages/browser-integration-tests/playwright.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ const config: PlaywrightTestConfig = {
},
],

reporter: process.env.CI ? [['line'], ['junit', { outputFile: 'results.junit.xml' }]] : 'list',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the CI environment variable is not set, this will default to 'list' reporter. Is this the intended behavior? If not, consider adding a default reporter.


globalSetup: require.resolve('./playwright.setup.ts'),
globalTeardown: require.resolve('./playwright.teardown.ts'),
};
Expand Down
2 changes: 1 addition & 1 deletion dev-packages/e2e-tests/test-applications/ember-classic/playwright.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const config: PlaywrightTestConfig = {
forbidOnly: !!process.env.CI,
retries: 0,
/* Reporter to use. See https://playwright.dev/docs/test-reporters */
reporter: 'list',
reporter: process.env.CI ? [['line'], ['junit', { outputFile: 'results.junit.xml' }]] : 'list',
Copy link

@codecov codecov bot Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same note for the CI environment variable applies here and for all other similar changes.

/* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */
use: {
/* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */
Expand Down
2 changes: 1 addition & 1 deletion dev-packages/e2e-tests/test-applications/ember-embroider/playwright.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const config: PlaywrightTestConfig = {
forbidOnly: !!process.env.CI,
retries: 0,
/* Reporter to use. See https://playwright.dev/docs/test-reporters */
reporter: 'list',
reporter: process.env.CI ? [['line'], ['junit', { outputFile: 'results.junit.xml' }]] : 'list',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same reporter switching logic is repeated. You should factor out this piece of code into a reusable function or a separate configuration file.

/* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */
use: {
/* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */
Expand Down
2 changes: 1 addition & 1 deletion dev-packages/e2e-tests/test-applications/node-express-send-to-sentry/playwright.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const config = {
/* Retry on CI only */
retries: 0,
/* Reporter to use. See https://playwright.dev/docs/test-reporters */
reporter: 'list',
reporter: process.env.CI ? [['line'], ['junit', { outputFile: 'results.junit.xml' }]] : 'list',
/* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */
use: {
/* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */
Expand Down
2 changes: 1 addition & 1 deletion dev-packages/e2e-tests/test-applications/react-send-to-sentry/playwright.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const config = {
/* Opt out of parallel tests on CI. */
workers: 1,
/* Reporter to use. See https://playwright.dev/docs/test-reporters */
reporter: 'list',
reporter: process.env.CI ? [['line'], ['junit', { outputFile: 'results.junit.xml' }]] : 'list',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar repetition of the reporter assignment based on environment variable 'CI'. This should be refactored into a utility function.

/* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */
use: {
/* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */
Expand Down
2 changes: 1 addition & 1 deletion dev-packages/test-utils/src/playwright-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export function getPlaywrightConfig(
/* In dev mode some apps are flaky, so we allow retry there... */
retries: testEnv === 'development' ? 3 : 0,
/* Reporter to use. See https://playwright.dev/docs/test-reporters */
reporter: process.env.CI ? 'line' : 'list',
reporter: process.env.CI ? [['line'], ['junit', { outputFile: 'results.junit.xml' }]] : 'list',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, similar repetition of the reporter assignment based on environment variable 'CI' as in line 129. Please consider creating a dedicated function or configurating module for this logic.

/* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */
use: {
/* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */
Expand Down
2 changes: 1 addition & 1 deletion packages/remix/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@
"test:integration:prepare": "(cd test/integration && yarn install)",
"test:integration:clean": "(cd test/integration && rimraf .cache node_modules build)",
"test:integration:client": "yarn playwright install-deps && yarn playwright test test/integration/test/client/ --project='chromium'",
"test:integration:client:ci": "yarn test:integration:client --reporter='line'",
"test:integration:client:ci": "yarn test:integration:client",
Copy link

@codecov codecov bot Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of the reporter from the test script has been noted. This is now handled by the test configuration file.

"test:integration:server": "export NODE_OPTIONS='--stack-trace-limit=25' && vitest run",
"test:unit": "jest",
"test:watch": "jest --watch",
Expand Down
1 change: 1 addition & 0 deletions packages/remix/playwright.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const config: PlaywrightTestConfig = {
},
// Run tests inside of a single file in parallel
fullyParallel: true,
reporter: process.env.CI ? [['line'], ['junit', { outputFile: 'results.junit.xml' }]] : 'list',
Copy link

@codecov codecov bot Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of 'junit' reporter for CI environment is a good addition. It will allow tests to generate JUnit style XML files, which can be used by various CI tools for detailed test reports.

// Use 3 workers on CI, else use defaults (based on available CPU cores)
// Note that 3 is a random number selected to work well with our CI setup
workers: process.env.CI ? 3 : undefined,
Expand Down
Loading