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

chore: Update c8 to merge v8 coverage reports asynchronously to avoid OOM issues #1652

Merged
merged 2 commits into from May 31, 2023

Conversation

bizob2828
Copy link
Member

@bizob2828 bizob2828 commented May 30, 2023

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

@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #1652 (2474bdb) into main (b693ba0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1652   +/-   ##
=======================================
  Coverage   96.68%   96.68%           
=======================================
  Files         200      200           
  Lines       39078    39078           
  Branches       24       24           
=======================================
  Hits        37781    37781           
  Misses       1297     1297           
Flag Coverage Δ
esm-unit-tests-14.x 47.80% <ø> (ø)
esm-unit-tests-16.x 92.11% <ø> (ø)
esm-unit-tests-18.x 92.11% <ø> (ø)
integration-tests-14.x 78.97% <ø> (ø)
integration-tests-16.x 79.07% <ø> (+0.01%) ⬆️
integration-tests-18.x 79.06% <ø> (-0.01%) ⬇️
unit-tests-14.x 91.30% <ø> (ø)
unit-tests-16.x 91.36% <ø> (ø)
unit-tests-18.x 91.34% <ø> (ø)
versioned-tests-14.x 75.60% <ø> (ø)
versioned-tests-16.x 76.90% <ø> (ø)
versioned-tests-18.x 76.90% <ø> (ø)

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

@jmartin4563 jmartin4563 self-assigned this May 31, 2023
Copy link
Contributor

@jmartin4563 jmartin4563 left a 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
Copy link
Contributor

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

Copy link
Contributor

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

https://github.com/newrelic/node-newrelic-koa/pull/148/files#diff-bdac2f5bfe1a2d5ea7f375061960ebb16d13f7541b4e1ebc2f30fc55b9975187

Copy link
Member Author

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

@bizob2828 bizob2828 merged commit 34376d7 into newrelic:main May 31, 2023
23 checks passed
Node.js Engineering Board automation moved this from Needs PR Review to Done: Issues recently completed May 31, 2023
@jmartin4563 jmartin4563 added dev:automation Indicates CI automation dev:tests Indicates only changes to tests labels May 31, 2023
@github-actions github-actions bot mentioned this pull request Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev:automation Indicates CI automation dev:tests Indicates only changes to tests
Projects
Node.js Engineering Board
  
Done: Issues recently completed
Development

Successfully merging this pull request may close these issues.

c8 runs out of memory on versioned tests runs
2 participants