-
Notifications
You must be signed in to change notification settings - Fork 404
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 c8 to merge v8 coverage reports asynchronously to avoid OOM issues #1652
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1652 +/- ##
=======================================
Coverage 96.68% 96.68%
=======================================
Files 200 200
Lines 39078 39078
Branches 24 24
=======================================
Hits 37781 37781
Misses 1297 1297
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Confirmed locally that running with minor flag and these changes that coverage successfully generates, while running with minor flag on main OOMs
@@ -9,6 +9,10 @@ VERSIONED_MODE="${VERSIONED_MODE:---minor}" | |||
SAMPLES="${SAMPLES:-10}" | |||
export NODE_OPTIONS="--max-old-space-size=4096" | |||
SKIP_C8="${SKIP_C8:-false}" | |||
# In CI we only want to run lcovonly | |||
# but when running locally we want to see the beautiful |
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.
👏🏻 thank you! i use the html reports all the 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.
Do we also want to unleash the koa tests now that c8 supports async merge? Currently they're locked down to a sample of 5, which still results in 128 runs with the minor flag
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 think it's fine to keep
Description
My work on c8 was released. This updates the workflows to always run versioned tests with c8 and also use the
--merge-async
flag to avoid the OOM issues during the report creation.You can see here it works with the minor flag now with no OOM issues: versioned tests with c8 and minor flag, versioned tests with c8 and major flag. Since we can always run c8 I got rid of the nightly versioned test upload. This will make coverage in codecov realtime.
Links
Closes #1616