-
Notifications
You must be signed in to change notification settings - Fork 506
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
chore: update lerna since parameter for push github action #1580
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1580 +/- ##
==========================================
- Coverage 96.06% 91.77% -4.29%
==========================================
Files 14 137 +123
Lines 914 7064 +6150
Branches 199 1424 +1225
==========================================
+ Hits 878 6483 +5605
- Misses 36 581 +545
|
@pichlermarc to fix #1473, for main branch should we run unit test for all package or just the change package introduce by new commit? |
.github/workflows/unit-test.yml
Outdated
- name: Unit tests | ||
run: npm run test:ci:changed -- ${{ matrix.lerna-extra-args }} | ||
- name: Unit Test | ||
run: npm run test:ci -- --since ${{github.event.pull_request.base.sha || github.event.push.before }} ${{ matrix.lerna-extra-args }} |
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 never fully understood how the code coverage works in details, but if I understand correctly, the goal is for pushes to main to always generate full report with all repo tests in codecov, where as now only web is reported:
In this case, wouldn't running the tests with --since github.event.push.before
will cause partial reports that only tracks the changed package in each merge to main, but will skip packages that have not had any change in the merge?
I added this flag long ago in #614 and the goal was to speed up ci time for PRs, but I guess for merges to main
it would be best to just run full tests without --since
. I wonder what are your thoughts and if you support it - are aware of a way to configure it in the yml
? We can also rethink if we want this optimization or just run all tests all the time (push to main and PRs). If I remember correctly, the"Unit test" CI step run time was few minutes, and not dramatic compared to overall workflow time. Since it confuses codecov, we can consider just removing the flag altogether.
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.
In this case, wouldn't running the tests with --since github.event.push.before will cause partial reports that only tracks the changed package in each merge to main, but will skip packages that have not had any change in the merge?
Yes, that also why I want to confirm this behavior with @pichlermarc
Codecov also support Flag feature with CarryForward, you can see one of the report here:
https://app.codecov.io/gh/open-telemetry/opentelemetry-js-contrib/commit/7b14c92d6913d104401ec086034168dfc57aedfe
In above commit, I configure to skip the node test but the codecov still capture the previous node's code coverage in single report.
But to fully support our codebase, each package must define it own flag/paths in codecov.yml which could become cumbersome.
If I remember correctly, the"Unit test" CI step run time was few minutes, and not dramatic compared to overall workflow time.
Yes, a full unit test is arround 5m, but it relative small compare to npm install + lerna bootstrap step which account for 30m of execution time.
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.
@blumamir I dig a little bit on to codecov's Components and Flags and think it could potential improve but would require more code change + some potential maintaince burden. I can create a prototype PR for that.
So I think for now it better to just do a full-run on node@14 to collect coverage report. For Node@16 and node@18 we can run a delta-run to speed thing up a bit, what do you think?
Another thought. If I understand correctly, running codecov on
Is it right? In this case, should both the PR commit and |
@blumamir Codecov seem to have a carry forward flag for this use case Let me test to see if this work |
…trib into chore-1473-main-codecov
This reverts commit 94da7ec.
…lemetry-js-contrib into chore-1473-main-codecov
…trib into chore-1473-main-codecov
closed in favor of #1605 |
Which problem is this PR solving?
Fixes #1473
--since origin/main
. This is correct for PR but not for commit that push on main since this will alway return empty change set.Short description of the changes