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.

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

feat(ci): Add junit reporters to jest/vitest, codecov test results #15341

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Feb 7, 2025

I noticed that not all of our tests were being tracked in codecov. This adds jest-junit reporter for our jest tests and configures vitest to use junit reporter (when CI env var is set).

This brings the number of tests tracked from ~700 to 4.5k

@billyvg billyvg added the ci-codecov-ai-review AI Review by Codecov label Feb 7, 2025
Copy link

@codecov codecov bot left a comment

Choose a reason for hiding this comment

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

The code changes predominantly center around integrating codecov reporting and 'jest-junit' into the project. While this is generally well done, there are some concerns to be addressed mainly regarding the error handling and reuse of code.

.github/workflows/build.yml Show resolved Hide resolved
continue-on-error: true
uses: codecov/test-results-action@v1
with:
files: packages/**/*.xml
Copy link

Choose a reason for hiding this comment

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

You have hard-coded the path to the test results. This could potentially cause problems when the project structure changes. Consider moving this into a configuration file or an environment variable so that any potential changes in the project structure are easier to manage.

.github/workflows/build.yml Show resolved Hide resolved
package.json Show resolved Hide resolved
@@ -10,6 +10,10 @@ export default defineConfig({
enabled: true,
reportsDirectory: './coverage',
},
reporters: ['default', 'junit'],
Copy link

Choose a reason for hiding this comment

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

You have chosen to use both the 'default' and 'junit' reporters. Ensure that using both these reporters together does not produce conflicting or duplicate reports.

version "16.0.0"
resolved "https://registry.yarnpkg.com/jest-junit/-/jest-junit-16.0.0.tgz#d838e8c561cf9fdd7eb54f63020777eee4136785"
integrity sha512-A94mmw6NfJab4Fg/BlvVOUXzXgF0XIH6EmTgJ5NDPp4xoKq0Kr7sErb+4Xs9nZvu58pJojz5RFGpqnZYJTrRfQ==
dependencies:
Copy link

Choose a reason for hiding this comment

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

The addition of the jest-junit package has introduced several new dependencies. Make sure these new dependancies do not cause conflicts with existing code or other packages.

Copy link

@codecov codecov bot left a comment

Choose a reason for hiding this comment

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

The current code diff reflects an effort to integrate jest-junit to the project for test reporting and uploading those test results to Codecov in different jobs. However, there are some improvements necessary to ensure a more ideal behavior.

.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
jest/jest.config.js Show resolved Hide resolved
package.json Show resolved Hide resolved
@@ -10,6 +10,10 @@ export default defineConfig({
enabled: true,
reportsDirectory: './coverage',
},
reporters: ['default', 'junit'],
Copy link

Choose a reason for hiding this comment

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

Ensure that the junit reporter does not conflict with other reporters. Also, make sure it's supported by the installed version of vitest.

@@ -10,6 +10,10 @@
enabled: true,
reportsDirectory: './coverage',
},
reporters: ['default', 'junit'],
outputFile: {
Copy link

Choose a reason for hiding this comment

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

Double check the jest configurations and make sure that outputFile is a valid configuration option, as it's not recognized in the base jest configuration.

Copy link

@codecov codecov bot left a 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 455666a

jest/jest.config.js Show resolved Hide resolved
@@ -7,7 +7,6 @@ export default defineConfig({
coverage: {},
globals: true,
setupFiles: ['./setup-test.ts'],
reporters: ['default'],
Copy link

Choose a reason for hiding this comment

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

The 'default' reporter was removed from some vitest configurations, but it might be beneficial to leave this for local development. If you're aiming to use Junit reporter on CI and default reporter locally, you might want to consider a conditionally loaded configuration based on the environment (development vs CI).

@@ -10,6 +10,10 @@ export default defineConfig({
enabled: true,
reportsDirectory: './coverage',
},
reporters: ['default', ...(process.env.CI ? ['junit'] : [])],
Copy link

Choose a reason for hiding this comment

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

The test results filename should probably include the names of the individual test suites to prevent them from overwriting each other. Consider dynamically generating the output filename based on the test suite name.

@billyvg billyvg removed the ci-codecov-ai-review AI Review by Codecov label Feb 7, 2025
continue-on-error: true
uses: codecov/test-results-action@v1
with:
directory: dev-packages/node-integration-tests
Copy link
Member Author

Choose a reason for hiding this comment

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

continue-on-error: true
uses: codecov/test-results-action@v1
with:
directory: packages/remix
Copy link
Member Author

Choose a reason for hiding this comment

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

continue-on-error: true
uses: codecov/test-results-action@v1
with:
files: packages/**/*.junit.xml
Copy link
Member Author

Choose a reason for hiding this comment

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

continue-on-error: true
uses: codecov/test-results-action@v1
with:
files: packages/**/*.junit.xml
Copy link
Member Author

Choose a reason for hiding this comment

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

@billyvg
Copy link
Member Author

billyvg commented Feb 7, 2025

This brings the number of tests tracked from ~700 to 4.5k

@billyvg billyvg marked this pull request as ready for review February 7, 2025 20:34
@billyvg billyvg requested a review from a team as a code owner February 7, 2025 20:34
@billyvg billyvg requested review from joseph-sentry, a team, chargome and s1gr1d and removed request for a team February 7, 2025 20:35
Copy link

@joseph-sentry joseph-sentry left a comment

Choose a reason for hiding this comment

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

lgtm from my end, just one optional enhancement you can make to the jest config to get some nice formatting for test names in the codecov UI

i see now that specifying the files using globs was necessary for the browser unit tests, although i'd expect the CLI to find those files, so i'll make an issue to look into that here

jest/jest.config.js Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 7, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
306 1 305 17
View the top 1 failed test(s) by shortest run time
transactions.test.ts Should send a transaction for instrumented server actions
Stack Traces | 4.01s run time
transactions.test.ts:53:5 Should send a transaction for instrumented server actions

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants