-
Notifications
You must be signed in to change notification settings - Fork 12
fix: Configure Codecov for mono-repo structure #318
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
Conversation
📝 WalkthroughWalkthroughThis pull request extracts test utilities from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #318 +/- ##
===========================================
+ Coverage 27.95% 91.18% +63.23%
===========================================
Files 87 97 +10
Lines 3291 3723 +432
Branches 1735 1905 +170
===========================================
+ Hits 920 3395 +2475
+ Misses 2371 328 -2043
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add ignore paths for examples, benchmark, and doc directories - Split coverage upload into separate steps with flags for each package - Associate each lcov.info file with its corresponding package flag
a0c4ed4 to
e1ad156
Compare
e764a28 to
87fc354
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yaml (1)
151-179: Coverage steps should not be gated by cancellation and need explicit timeouts.
The new&& !cancelled()conditions and missingtimeout-minutesconflict with the requirement that coverage steps run to completion with specified timeouts.✅ Suggested adjustment
- name: Run Tests With Coverage if: ${{ matrix.os == 'ubuntu-latest' && matrix.dart_sdk == '3.8.1' && matrix.deps == 'upgrade' }} + timeout-minutes: 30 run: | dart pub global activate coverage melos run coverage --no-select - name: Upload Coverage (relic_core) uses: codecov/codecov-action@v5 - if: ${{ matrix.os == 'ubuntu-latest' && matrix.dart_sdk == '3.8.1' && matrix.deps == 'upgrade' && !cancelled() }} + if: ${{ matrix.os == 'ubuntu-latest' && matrix.dart_sdk == '3.8.1' && matrix.deps == 'upgrade' }} + timeout-minutes: 10 with: token: ${{ secrets.CODECOV_TOKEN }} files: packages/relic_core/coverage/lcov.info flags: relic_core - name: Upload Coverage (relic_io) uses: codecov/codecov-action@v5 - if: ${{ matrix.os == 'ubuntu-latest' && matrix.dart_sdk == '3.8.1' && matrix.deps == 'upgrade' && !cancelled() }} + if: ${{ matrix.os == 'ubuntu-latest' && matrix.dart_sdk == '3.8.1' && matrix.deps == 'upgrade' }} + timeout-minutes: 10 with: token: ${{ secrets.CODECOV_TOKEN }} files: packages/relic_io/coverage/lcov.info flags: relic_io - name: Upload Coverage (relic) uses: codecov/codecov-action@v5 - if: ${{ matrix.os == 'ubuntu-latest' && matrix.dart_sdk == '3.8.1' && matrix.deps == 'upgrade' && !cancelled() }} + if: ${{ matrix.os == 'ubuntu-latest' && matrix.dart_sdk == '3.8.1' && matrix.deps == 'upgrade' }} + timeout-minutes: 10 with: token: ${{ secrets.CODECOV_TOKEN }} files: packages/relic/coverage/lcov.info flags: relicAs per coding guidelines, coverage steps must run to completion with explicit timeouts.
🤖 Fix all issues with AI agents
In `@packages/relic_io/test/util/test_util.dart`:
- Around line 6-7: The export in packages/relic_io/test/util/test_util.dart is
self-referential; update the export statement so it re-exports the intended
external utilities (not itself) — replace the current "../util/test_util.dart"
export with the correct package export (for example
"package:test_utils/test_util.dart" or the appropriate package that actually
contains the shared test utilities) so the file re-exports the external
utilities rather than itself.
🧹 Nitpick comments (2)
pubspec.yaml (1)
52-52: Consider usingdart runinstead of deprecateddart pub run.
dart pub runhas been deprecated in favor ofdart run. While it still works, updating to the modern syntax is recommended.Suggested change
- - dart pub run melos run test + - dart run melos run testpackages/test_utils/lib/src/test_utils_base.dart (1)
137-145:actualis evaluated at test definition time, not execution time.In
singleTest, theactualparameter is evaluated when the test is defined, not when it runs. This could lead to unexpected behavior if callers expect lazy evaluation (e.g.,singleTest('test', computeValue(), expected)computes immediately).If this is intentional for simple constant checks, consider documenting this behavior. Otherwise, consider accepting a callback:
Alternative with lazy evaluation
`@isTest` void singleTest( final String description, - final dynamic actual, + final dynamic Function() actual, final dynamic expected, ) { test(description, () { - expect(actual, expected); + expect(actual(), expected); }); }Usage would change to:
singleTest('test', () => computeValue(), expected)
marcelomendoncasoares
left a comment
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.
LGTM!
Description
Codecov is reporting incorrect coverage (28%) after the recent mono-repo restructuring, when the actual coverage is above 90%. This PR fixes the Codecov configuration to properly handle the multi-package mono-repo structure.
test_utilspackage containing shared test utilities (parameterizedTest,makeSimpleRequest,syncHandler, etc.)relic_corerelic_iorelicas they are implemented as integration tests (will require a bigger refactor to split)dart-tests.yamltoci.yamlrelic_core,relic_io,relic)coveragescript for local coverage collectionRelated Issues
Pre-Launch Checklist
///), ensuring consistency with existing project documentation.Breaking Changes
Additional Notes
The test reorganization moves tests closer to the code they test, improving maintainability and making it clearer which package owns which functionality. The new
test_utilspackage is internal (not published) and provides shared testing infrastructure across all packages.Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.