restore code coverage #4841

mcollina · 2023-06-21T10:24:30Z

After #4691 landed, code coverage has been broken across all builds.
This PR fixes it by:

working around a memory leak in Node.js v20 which forces us to call .close() every time. Hopefully this will be fixed in v20.5, but we will see.
Moving away from nyc and adopting c8 instead.

Checklist

run npm run test and npm run benchmark
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina · 2023-06-25T09:36:42Z

PTAL, this should be good to go.

Signed-off-by: Matteo Collina <hello@matteocollina.com>

Fdawgs · 2023-06-25T10:07:41Z

This is to accommodate Node 20 changes, right?

mcollina · 2023-06-25T10:09:47Z

If it passes, yes.

mcollina · 2023-07-06T21:42:40Z

We have made progress I think. With 20.4.0 we should be able to move this forward.

package.json

mcollina · 2023-07-07T08:04:10Z

Note that we dropped test coverage, so we should update those checks.

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina · 2023-07-09T13:55:58Z

The problem with supporting Node v20 is that tap and nyc do not generate code coverage reliably.

Ultimately we should switch to using c8 for code coverage - I don't think we would be able to get any of it working on top of nyc.

Signed-off-by: Matteo Collina <hello@matteocollina.com>

package.json

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina · 2023-07-12T08:40:30Z

PTAL, this is ready and passing

Uzlopak

LGTM

package.json

.taprc

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

jsumners

I wish we could get some community involvement to progress tapjs/tapjs#821 and get c8 in tap.

jsumners · 2023-07-12T11:51:26Z

fastify.js

@@ -350,7 +350,8 @@ function fastify (options) {
    listeningOrigin: {
      get () {
        const address = this.addresses().slice(-1).pop()
-        /* istanbul ignore if windows: unix socket is not testable on Windows platform */
+        /* ignore if windows: unix socket is not testable on Windows platform */
+        /* c8 ignore next 3 */


🤢 c8 doesn't support specifying the exception to ignore? It only supports next and next N lines? That is going to be a real pain to maintain as code evolves.

Maybe it got smarter over time, not sure

No, the docs indicate it is only those two possibilities.

This reverts commit 7bcfd69.

mcollina · 2023-07-12T12:58:50Z

PTAL @Eomm

isaacs · 2023-07-12T23:20:53Z

@jsumners

I wish we could get some community involvement to progress tapjs/tapjs#821 and get c8 in tap.

It's progressing as fast as I can progress it lol 😅 But if anyone would like to help out, I've definitely got some tasks that could use additional hands.

I've got tests passing for 90% of the subpackages now, just 3 remaining, should be done in the next week or so. Remaining work is deployment (ie, write a script that will bump and set internal dependency versions appropriately, publish anything that's changed, and bundle the appropriate dependencies of the top-level tap package), at which point you can definitely start using it in beta, and then docs and website, at which point it'll be "released".

It's not quite accurate to say that the goal is "get c8 in tap". Because C8 just sets the node v8 coverage env, it covers ALL javascript run by a program. In a typical run, this means that upwards from 99% of the contents of the coverage dir is going to be coming from node internals and deps in node_modules. Tap is hitting the v8 inspector API directly, and so it can track coverage information about the just files in your actual program. I've also been putting it through pretty demanding play testing to ensure that coverage maps always just work, no matter what combination of esm, jsx, typescript, etc. is in use.

The /* c8 ignore ... */ comments are implemented in the reporter, specifically in the v8-to-istanbul package. Eventually I do want to probably fork away from the C8 reporter and refactor all of that logic so that I can get more control over how reports get generated. C8 is very tied up in yargs patterns, and not nearly as flexible as I'd like. (For example, to get the coverage reporting to be in a reasonable spot in the output, I'm having to hijack console.error and capture the results, because there's no other way to just get a string of the report output.) Platform-specific ignores are a feature I'm also really missing.

jsumners · 2023-07-12T23:27:31Z

It's progressing as fast as I can progress it lol 😅

Sorry if I offended. I didn't mean to. We are well aware of maintenance burden and limited time. I admit, I didn't peruse the latest updates on that issue before linking. It sounds like you have made a lot of progress recently and I look forward to the result.

If I manage to overcome some personal stresses, I'll see what I can do to help. But my main task right now is to find gainful employment.

isaacs · 2023-07-13T01:09:09Z

Oh! No offense taken. Just being playful, sometimes that doesn't come across in text, sorry ;)

jsumners · 2023-07-13T01:36:51Z

Oh! No offense taken. Just being playful, sometimes that doesn't come across in text, sorry ;)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [fastify](https://www.fastify.io/) ([source](https://togithub.com/fastify/fastify)) | [`4.19.2` -> `4.20.0`](https://renovatebot.com/diffs/npm/fastify/4.19.2/4.20.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/fastify/4.20.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fastify/4.20.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fastify/4.19.2/4.20.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fastify/4.19.2/4.20.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>fastify/fastify (fastify)</summary> ### [`v4.20.0`](https://togithub.com/fastify/fastify/releases/tag/v4.20.0) [Compare Source](https://togithub.com/fastify/fastify/compare/v4.19.2...v4.20.0) #### What's Changed - build(deps-dev): Bump [@sinclair/typebox](https://togithub.com/sinclair/typebox) from 0.28.20 to 0.29.1 by [@dependabot](https://togithub.com/dependabot) in [fastify/fastify#4877 - Update Prototype-Poisoning.md by [@ed-henrique](https://togithub.com/ed-henrique) in [fastify/fastify#4879 - docs: adjust line for linter by [@metcoder95](https://togithub.com/metcoder95) in [fastify/fastify#4882 - Fixed type inference on .code().send() by [@aadito123](https://togithub.com/aadito123) in [fastify/fastify#4880 - chore: refactor Error-typings and tests by [@svrnwnsch](https://togithub.com/svrnwnsch) in [fastify/fastify#4824 - test: add missing assertion for a test by [@kibertoad](https://togithub.com/kibertoad) in [fastify/fastify#4701 - chore: tests for genReqIdFactory and minor changes by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4783 - Added type definition for allowUnsafeRegex by [@xijdk](https://togithub.com/xijdk) in [fastify/fastify#4792 - docs(security): update policy on ci/cd reports by [@jsumners](https://togithub.com/jsumners) in [fastify/fastify#4890 - improve hooks.validate by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4804 - ci: enable caching; split jobs by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4889 - docs(plugins): note side-effect when using `await` in `fastify.register()` by [@hateablestream](https://togithub.com/hateablestream) in [fastify/fastify#4846 - \[hooks] Refine hook runners by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4805 - docs: fix wrong header size for FST_ERR_ASYNC_CONSTRAINT in Errors.md by [@graphiteisaac](https://togithub.com/graphiteisaac) in [fastify/fastify#4893 - fix: rework FastifyErrors, ensure documentation completeness by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4892 - feat: add childLoggerFactory config by [@brettwillis](https://togithub.com/brettwillis) in [fastify/fastify#4760 - Fix setNotFoundHandler handler type by [@Ethan-Arrowood](https://togithub.com/Ethan-Arrowood) in [fastify/fastify#4897 - restore code coverage by [@mcollina](https://togithub.com/mcollina) in [fastify/fastify#4841 - fix: handle abort signal before server is ready by [@shayff](https://togithub.com/shayff) in [fastify/fastify#4886 - docs(client-aborted): remove deprecated function by [@JekRock](https://togithub.com/JekRock) in [fastify/fastify#4898 - Revert "docs(client-aborted): remove deprecated function ([#4898](https://togithub.com/fastify/fastify/issues/4898))" by [@climba03003](https://togithub.com/climba03003) in [fastify/fastify#4901 - docs(logging): fix typo by [@yamanidev](https://togithub.com/yamanidev) in [fastify/fastify#4905 - Support IPv6 ::1 in listeningOrigin by [@mcollina](https://togithub.com/mcollina) in [fastify/fastify#4902 - fix: extend isCustomValidatorCompiler from parent controller by [@tinchoz49](https://togithub.com/tinchoz49) in [fastify/fastify#4903 - fix: allow uppercase requestdHeader by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4906 #### New Contributors - [@ed-henrique](https://togithub.com/ed-henrique) made their first contribution in [fastify/fastify#4879 - [@svrnwnsch](https://togithub.com/svrnwnsch) made their first contribution in [fastify/fastify#4824 - [@xijdk](https://togithub.com/xijdk) made their first contribution in [fastify/fastify#4792 - [@hateablestream](https://togithub.com/hateablestream) made their first contribution in [fastify/fastify#4846 - [@graphiteisaac](https://togithub.com/graphiteisaac) made their first contribution in [fastify/fastify#4893 - [@shayff](https://togithub.com/shayff) made their first contribution in [fastify/fastify#4886 - [@JekRock](https://togithub.com/JekRock) made their first contribution in [fastify/fastify#4898 - [@yamanidev](https://togithub.com/yamanidev) made their first contribution in [fastify/fastify#4905 - [@tinchoz49](https://togithub.com/tinchoz49) made their first contribution in [fastify/fastify#4903 **Full Changelog**: fastify/fastify@v4.19.2...v4.20.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/tomacheese/telcheck).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [fastify](https://www.fastify.io/) ([source](https://togithub.com/fastify/fastify)) | [`4.19.2` -> `4.20.0`](https://renovatebot.com/diffs/npm/fastify/4.19.2/4.20.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/fastify/4.20.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fastify/4.20.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fastify/4.19.2/4.20.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fastify/4.19.2/4.20.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>fastify/fastify (fastify)</summary> ### [`v4.20.0`](https://togithub.com/fastify/fastify/releases/tag/v4.20.0) [Compare Source](https://togithub.com/fastify/fastify/compare/v4.19.2...v4.20.0) #### What's Changed - build(deps-dev): Bump [@sinclair/typebox](https://togithub.com/sinclair/typebox) from 0.28.20 to 0.29.1 by [@dependabot](https://togithub.com/dependabot) in [fastify/fastify#4877 - Update Prototype-Poisoning.md by [@ed-henrique](https://togithub.com/ed-henrique) in [fastify/fastify#4879 - docs: adjust line for linter by [@metcoder95](https://togithub.com/metcoder95) in [fastify/fastify#4882 - Fixed type inference on .code().send() by [@aadito123](https://togithub.com/aadito123) in [fastify/fastify#4880 - chore: refactor Error-typings and tests by [@svrnwnsch](https://togithub.com/svrnwnsch) in [fastify/fastify#4824 - test: add missing assertion for a test by [@kibertoad](https://togithub.com/kibertoad) in [fastify/fastify#4701 - chore: tests for genReqIdFactory and minor changes by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4783 - Added type definition for allowUnsafeRegex by [@xijdk](https://togithub.com/xijdk) in [fastify/fastify#4792 - docs(security): update policy on ci/cd reports by [@jsumners](https://togithub.com/jsumners) in [fastify/fastify#4890 - improve hooks.validate by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4804 - ci: enable caching; split jobs by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4889 - docs(plugins): note side-effect when using `await` in `fastify.register()` by [@hateablestream](https://togithub.com/hateablestream) in [fastify/fastify#4846 - \[hooks] Refine hook runners by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4805 - docs: fix wrong header size for FST_ERR_ASYNC_CONSTRAINT in Errors.md by [@graphiteisaac](https://togithub.com/graphiteisaac) in [fastify/fastify#4893 - fix: rework FastifyErrors, ensure documentation completeness by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4892 - feat: add childLoggerFactory config by [@brettwillis](https://togithub.com/brettwillis) in [fastify/fastify#4760 - Fix setNotFoundHandler handler type by [@Ethan-Arrowood](https://togithub.com/Ethan-Arrowood) in [fastify/fastify#4897 - restore code coverage by [@mcollina](https://togithub.com/mcollina) in [fastify/fastify#4841 - fix: handle abort signal before server is ready by [@shayff](https://togithub.com/shayff) in [fastify/fastify#4886 - docs(client-aborted): remove deprecated function by [@JekRock](https://togithub.com/JekRock) in [fastify/fastify#4898 - Revert "docs(client-aborted): remove deprecated function ([#4898](https://togithub.com/fastify/fastify/issues/4898))" by [@climba03003](https://togithub.com/climba03003) in [fastify/fastify#4901 - docs(logging): fix typo by [@yamanidev](https://togithub.com/yamanidev) in [fastify/fastify#4905 - Support IPv6 ::1 in listeningOrigin by [@mcollina](https://togithub.com/mcollina) in [fastify/fastify#4902 - fix: extend isCustomValidatorCompiler from parent controller by [@tinchoz49](https://togithub.com/tinchoz49) in [fastify/fastify#4903 - fix: allow uppercase requestdHeader by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4906 #### New Contributors - [@ed-henrique](https://togithub.com/ed-henrique) made their first contribution in [fastify/fastify#4879 - [@svrnwnsch](https://togithub.com/svrnwnsch) made their first contribution in [fastify/fastify#4824 - [@xijdk](https://togithub.com/xijdk) made their first contribution in [fastify/fastify#4792 - [@hateablestream](https://togithub.com/hateablestream) made their first contribution in [fastify/fastify#4846 - [@graphiteisaac](https://togithub.com/graphiteisaac) made their first contribution in [fastify/fastify#4893 - [@shayff](https://togithub.com/shayff) made their first contribution in [fastify/fastify#4886 - [@JekRock](https://togithub.com/JekRock) made their first contribution in [fastify/fastify#4898 - [@yamanidev](https://togithub.com/yamanidev) made their first contribution in [fastify/fastify#4905 - [@tinchoz49](https://togithub.com/tinchoz49) made their first contribution in [fastify/fastify#4903 **Full Changelog**: fastify/fastify@v4.19.2...v4.20.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/redwoodjs/redwood).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

isaacs · 2023-08-06T16:25:16Z

Update on this: tap 18 is now available in alpha status. npm install tap@pre will get the new stuff. Docs and website are lacking, but tap -h is accurate and the code completions will steer you in the right direction.

jsumners · 2023-08-06T16:44:39Z

Awesome! Thank you @isaacs.

mcollina added 3 commits June 21, 2023 12:23

fixup

070dfa8

Signed-off-by: Matteo Collina <hello@matteocollina.com>

fixup

9a98902

Signed-off-by: Matteo Collina <hello@matteocollina.com>

Merge branch 'main' into no-warnings2

ba8d62c

mcollina requested review from Fdawgs and Uzlopak June 25, 2023 09:36

fixup

c20eeba

Signed-off-by: Matteo Collina <hello@matteocollina.com>

Fdawgs changed the title ~~Use a real node option instead of a bogus one~~ test: use a real node option instead of a bogus one Jul 2, 2023

climba03003 and others added 2 commits July 6, 2023 02:31

Merge branch 'main' into no-warnings2

ff03817

Merge branch 'main' into no-warnings2

b0a170c

Eomm reviewed Jul 7, 2023

View reviewed changes

package.json Show resolved Hide resolved

mcollina added 2 commits July 7, 2023 11:33

some improvements

ad8c8b3

Signed-off-by: Matteo Collina <hello@matteocollina.com>

Disable code coverage

fff4ea8

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina and others added 2 commits July 10, 2023 12:46

Moved from nyc to c8

ba39200

Signed-off-by: Matteo Collina <hello@matteocollina.com>

Merge branch 'main' into no-warnings2

c26efdb

Uzlopak reviewed Jul 10, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

Uzlopak and others added 2 commits July 10, 2023 12:54

Apply suggestions from code review

70f6000

fixup

8ecdd67

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina mentioned this pull request Jul 12, 2023

ci is flaky #4888

Closed

2 tasks

mcollina changed the title ~~test: use a real node option instead of a bogus one~~ restore code coverage Jul 12, 2023

Merge branch 'main' into no-warnings2

7282d57

Uzlopak approved these changes Jul 12, 2023

View reviewed changes

Uzlopak linked an issue Jul 12, 2023 that may be closed by this pull request

ci is flaky #4888

Closed

2 tasks

Eomm reviewed Jul 12, 2023

View reviewed changes

package.json Show resolved Hide resolved

package.json Show resolved Hide resolved

Eomm reviewed Jul 12, 2023

View reviewed changes

.taprc Show resolved Hide resolved

mcollina and others added 2 commits July 12, 2023 13:28

Update .taprc

3a6e728

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

Update package.json

7bcfd69

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

jsumners reviewed Jul 12, 2023

View reviewed changes

Revert "Update package.json"

79be7f2

This reverts commit 7bcfd69.

Eomm approved these changes Jul 12, 2023

View reviewed changes

Eomm merged commit 53164c1 into main Jul 12, 2023
25 checks passed

Eomm deleted the no-warnings2 branch July 12, 2023 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restore code coverage #4841

restore code coverage #4841

mcollina commented Jun 21, 2023 •

edited

mcollina commented Jun 25, 2023

Fdawgs commented Jun 25, 2023

mcollina commented Jun 25, 2023

mcollina commented Jul 6, 2023

mcollina commented Jul 7, 2023

mcollina commented Jul 9, 2023

mcollina commented Jul 12, 2023

Uzlopak left a comment

jsumners left a comment

jsumners Jul 12, 2023

mcollina Jul 12, 2023

jsumners Jul 12, 2023

mcollina commented Jul 12, 2023

isaacs commented Jul 12, 2023 •

edited

jsumners commented Jul 12, 2023

isaacs commented Jul 13, 2023

jsumners commented Jul 13, 2023

isaacs commented Aug 6, 2023

jsumners commented Aug 6, 2023

restore code coverage #4841

restore code coverage #4841

Conversation

mcollina commented Jun 21, 2023 • edited

Checklist

mcollina commented Jun 25, 2023

Fdawgs commented Jun 25, 2023

mcollina commented Jun 25, 2023

mcollina commented Jul 6, 2023

mcollina commented Jul 7, 2023

mcollina commented Jul 9, 2023

mcollina commented Jul 12, 2023

Uzlopak left a comment

Choose a reason for hiding this comment

jsumners left a comment

Choose a reason for hiding this comment

jsumners Jul 12, 2023

Choose a reason for hiding this comment

mcollina Jul 12, 2023

Choose a reason for hiding this comment

jsumners Jul 12, 2023

Choose a reason for hiding this comment

mcollina commented Jul 12, 2023

isaacs commented Jul 12, 2023 • edited

jsumners commented Jul 12, 2023

isaacs commented Jul 13, 2023

jsumners commented Jul 13, 2023

isaacs commented Aug 6, 2023

jsumners commented Aug 6, 2023

mcollina commented Jun 21, 2023 •

edited

isaacs commented Jul 12, 2023 •

edited