-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
build: Add codecov test analytics for playwright tests #13654
Conversation
.github/workflows/build.yml
Outdated
@@ -610,6 +610,13 @@ jobs: | |||
overwrite: true | |||
retention-days: 7 | |||
|
|||
- name: Upload test results to Codecov | |||
if: ${{ !cancelled() }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if: ${{ !cancelled() }} | |
if: cancelled() == false |
I think that's enough here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied the snippet from https://docs.codecov.com/docs/test-result-ingestion-beta#2-add-the-script-codecovtest-results-actionv1-to-your-ci-yaml-file
but I think you're right, this feels more correct to me. Will switch!
Looks like it works! https://app.codecov.io/gh/getsentry/sentry-javascript/tests/abhi-playwright-codecov-test-analytics (make sure you're logged in) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code contains Git diff changes which primarily aim at integrating codecov test results actions in GitHub Workflows, and modifying test reporters for CI. The changes are well implemented, but are redundant and hard-coded, which might raise maintenance issues in the future. Moreover, the usage of tokens can be risky and should be carefully verified.
uses: codecov/test-results-action@v1 | ||
with: | ||
directory: dev-packages/browser-integration-tests | ||
token: ${{ secrets.CODECOV_TOKEN }} |
There was a problem hiding this comment.
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.
- name: Upload test results to Codecov | ||
if: cancelled() == 'false' | ||
uses: codecov/test-results-action@v1 | ||
with: |
There was a problem hiding this comment.
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.
@@ -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', |
There was a problem hiding this comment.
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.
@@ -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', |
There was a problem hiding this comment.
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.
@@ -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', |
There was a problem hiding this comment.
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.
@@ -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', |
There was a problem hiding this comment.
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.
@@ -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", |
There was a problem hiding this comment.
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.
@@ -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', |
There was a problem hiding this comment.
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.
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodecovAI submitted a new review for b4228b8
@@ -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", |
There was a problem hiding this comment.
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.
@@ -30,6 +30,8 @@ const config: PlaywrightTestConfig = { | |||
}, | |||
], | |||
|
|||
reporter: process.env.CI ? [['line'], ['junit', { outputFile: 'results.junit.xml' }]] : 'list', |
There was a problem hiding this comment.
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.
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
https://docs.codecov.com/docs/test-result-ingestion-beta
https://playwright.dev/docs/test-reporters
Adds codecov test analytics to the repo, specifically for our playwright tests.
This works by using the junit reporter with playwright, and then uploading that via the
codecov/test-results-action@v1
GitHub Action.