feat: add codecov to the project #3168
Conversation
vishvamsinh28
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
✅ Deploy Preview for asyncapi-website ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3168--asyncapi-website.netlify.app/ |
akshatnema
added
the
gsoc
|
Is there a way we can see the codecov coverage report before merging this PR? |
I tried testing it by opening a PR in my forked repo, but it didn't work, so I guess not 😅. However, based on this comment, I think we're good to go. |
There was a problem hiding this comment.
@vishvamsinh28 Add following things in the PR:
- Add proper PR description on why codecov is being added and how it will be used.
- Add test runs for this branch in your forked PR and add the test workflow links in PR description.
|
Added links to my PRs in forked repo |
|
@vishvamsinh28 can you please provide the PR link where we can have a look on the working of this feature |
asyncapi-bot
requested review from
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
|
/update |
| @@ -1,7 +1,8 @@ | |||
| module.exports = { | |||
| verbose: true, | |||
| collectCoverage: true, | |||
| collectCoverageFrom: ['scripts/**/*.js'], | |||
There was a problem hiding this comment.
We don't need that jest will only include files that are directly imported in our test files for coverage.
There was a problem hiding this comment.
That's the case. Jest is now taking fixture files also into coverage, and that's wrong.
There was a problem hiding this comment.
I will update it for excluding fixture files
WalkthroughThe pull request introduces modifications to the workflow for testing Node.js projects. Key changes include the addition of steps for installing dependencies, running tests, linting, generating release assets, and uploading coverage reports. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/if-nodejs-pr-testing.yml (1)
Line range hint
69-71: Consider running linter on all platformsThe addition of a linting step is excellent for maintaining code quality. However, currently, it's set to run only on
ubuntu-latest. Consider running the linter on all platforms to ensure consistent code style across different environments.- if: steps.packagejson.outputs.exists == 'true' name: Run linter run: npm run lint --if-presentThis change would ensure that linting is performed regardless of the operating system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/if-nodejs-pr-testing.yml (2 hunks)
- jest.config.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
jest.config.js (2)
4-5: LGTM: Coverage reporting configuration looks good.The addition of
coverageReportersandcoverageDirectoryis appropriate for integrating Codecov:
- The selected reporters ('text', 'lcov', 'json-summary') provide a good balance of human-readable and machine-readable outputs.
- Setting the coverage directory to 'coverage' follows common conventions.
These changes will enable detailed coverage reporting, supporting the PR's objective of enhancing visibility of test coverage.
9-9: LGTM: Newline added as requested.The addition of a newline at the end of the file is a good practice and addresses the previous comment by sambhavgupta0705. This change should resolve any related linting issues.
.github/workflows/if-nodejs-pr-testing.yml (5)
63-65: LGTM: Dependency installation step addedThe addition of the "Install dependencies" step using
npm ciis a good practice for CI environments. It ensures that all required dependencies are installed consistently based on the package-lock.json file, promoting reproducible builds.
Line range hint
66-68: LGTM: Test step improvedThe modification to the "Test" step is an improvement. By checking for the existence of
package.jsonand using the--if-presentflag, the workflow becomes more robust and flexible, accommodating projects that may not have a test script defined.
Line range hint
72-75: LGTM: Release assets generation step addedThe addition of the "Run release assets generation" step is a good safeguard to ensure that PRs don't inadvertently break the release process. The use of the
--if-presentflag makes this step flexible for projects that may or may not have this script defined.
77-84: LGTM: Codecov integration implemented correctlyThe addition of the Codecov upload step is well-implemented and aligns with the PR objectives. Good practices observed:
- Using a specific commit hash for the codecov action.
- Correctly referencing the
CODECOV_TOKENfrom secrets.- Specifying the coverage file path.
The
flagparameter has been removed as suggested in the past comments, which is the correct approach for this use case.
Line range hint
63-84: Overall assessment: Significant improvements to the CI workflowThe changes made to this workflow file significantly enhance the CI process for Node.js projects:
- Dependency installation ensures consistent environments.
- Conditional test and lint steps improve flexibility.
- Release asset generation safeguards the release process.
- Codecov integration provides valuable coverage insights.
These additions align well with the PR objectives and follow good CI practices. The workflow is now more robust and informative, which will contribute to maintaining and improving code quality in the project.
Consider the suggestion to run the linter on all platforms for more comprehensive code style checks.
| @@ -1,7 +1,9 @@ | |||
| module.exports = { | |||
| verbose: true, | |||
| collectCoverage: true, | |||
| coverageReporters: ['text', 'lcov', 'json-summary'], | |||
| coverageDirectory: 'coverage', | |||
| collectCoverageFrom: ['scripts/**/*.js'], | |||
There was a problem hiding this comment.
💡 Codebase verification
Coverage reports are not being generated.
Despite collectCoverage being enabled in jest.config.js, the coverage directory does not exist. This indicates that coverage reports are not being generated as expected.
Please ensure that:
- Tests are executed with coverage collection enabled.
- The test scripts are correctly configured to generate coverage reports.
- There are no errors during test execution that prevent coverage generation.
🔗 Analysis chain
Verify coverage collection configuration.
The current collectCoverageFrom configuration includes only 'scripts/**/*.js'. Based on the previous discussion in the comments, there was a concern about including 'fixture' files in the coverage reports.
Please verify if the current configuration sufficiently excludes 'fixture' files from coverage reports. If not, consider updating it to explicitly exclude these files.
To check if 'fixture' files are being included in coverage reports, you can run the following command after running your tests:
If 'fixture' files are found in the coverage report and you want to exclude them, you might want to update the collectCoverageFrom configuration to something like:
collectCoverageFrom: ['scripts/**/*.js', '!**/fixtures/**']This will include all .js files in the 'scripts' directory and its subdirectories, but exclude any files in 'fixtures' directories.
</details>
<details>
<summary>🏁 Scripts executed</summary>
The following scripts were executed for the analysis:
Script:
```shell
#!/bin/bash
# Description: Check if 'fixture' files are included in coverage reports
# Look for 'fixture' files in the coverage report
grep -r "fixture" coverage/lcov-report
# If any 'fixture' files are found, they will be listed.
# If no output is produced, it means no 'fixture' files are included in the coverage report.
Length of output: 92
Script:
#!/bin/bash
# Description: Check if coverage directory exists and list its contents
if [ -d "coverage" ]; then
echo "Coverage directory exists. Listing contents:"
ls -la coverage
else
echo "Coverage directory does not exist."
fi
Length of output: 100
Script:
#!/bin/bash
# Description: Verify if coverage collection is enabled in jest.config.js
if grep -q "collectCoverage: true" jest.config.js; then
echo "Coverage collection is enabled in jest.config.js."
else
echo "Coverage collection is NOT enabled in jest.config.js."
fi
Length of output: 157
Script:
#!/bin/bash
# Description: Display the current collectCoverageFrom configuration in jest.config.js
grep "collectCoverageFrom" jest.config.js || echo "collectCoverageFrom is not defined in jest.config.js."
Length of output: 85
Script:
#!/bin/bash
# Description: Check if coverageDirectory is specified in jest.config.js
grep "coverageDirectory" jest.config.js || echo "coverageDirectory is not defined in jest.config.js. Default coverage directory is 'coverage'."
Length of output: 72
Script:
#!/bin/bash
# Description: Search for any coverage report directories other than the default 'coverage'
# Common coverage report directories
coverage_dirs=("coverage" "coverage-report" "lcov-report" "reports/coverage")
for dir in "${coverage_dirs[@]}"; do
if [ -d "$dir" ]; then
echo "Found coverage directory: $dir"
ls -la "$dir"
fi
done
# Additionally, search for any directories containing 'lcov' or 'coverage' in their names
fd --type d --glob '*coverage*' --glob '*lcov*' .
Length of output: 430
Script:
#!/bin/bash
# Description: Verify if tests have been executed to generate coverage reports
# Check if jest has been run by looking for recent test-related scripts in package.json
grep '"test"' package.json || echo "No test scripts found in package.json."
# Alternatively, check Git history for recent test executions
git log -n 1 --pretty=format:"%H" -- "**/*.test.js" || echo "No recent test commits found."
# Check for recent modifications in jest.config.js
git log -n 1 --pretty=format:"%H" jest.config.js || echo "No recent changes to jest.config.js found."
Length of output: 248
There was a problem hiding this comment.
coverage directory has been added in the .gitignore, hence it won't be part of repo. The coverage is generated everytime during github action run.
There was a problem hiding this comment.
@akshatnema, thank you for the clarification. I understand that the coverage directory is gitignored and coverage reports are generated during the GitHub Actions run. That makes sense.
(_/)
(o.O)
✏️ Learnings added
Learnt from: akshatnema
PR: asyncapi/website#3168
File: jest.config.js:6-6
Timestamp: 2024-10-15T17:44:59.028Z
Learning: In this repository, the `coverage` directory is added to `.gitignore`, and coverage reports are generated during GitHub Actions runs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
/rtm |
This PR integrates Codecov into our CI workflow to enhance the visibility of our test coverage across the codebase. By adding Codecov, we aim to:
Codecov will provide detailed reports on which lines of code are being covered by our tests and highlight any gaps in coverage. This helps identify areas that require additional testing.
With Codecov, every PR will now include a comment indicating the impact on test coverage. This ensures that as new features or bug fixes are added, we can maintain or improve the test coverage, ensuring code quality remains high.
How Codecov Will Be Used:
Codecov is integrated with the CI workflow. Once tests are run, the coverage reports will be uploaded to Codecov.
Codecov setup in forked repe :- vishvamsinh28#7
Codecov report for tests in forked repo :- vishvamsinh28#8
Summary by CodeRabbit
New Features
Bug Fixes
package.jsonfile is present, preventing errors in environments without it.