worker: improve code coverage by ErickWendel · Pull Request #41818 · nodejs/node · GitHub

It adds tests for the worker getHeadSnapshot function and assignEnvironmentData
Refs: lib/internal/worker.js.html#L412 and lib/internal/worker.js.html#L114

… in order

It's useful (or at least I think it is!) in PRs to increase code coverage to include a Refs: line showing what you are adding coverage for. For example:

Refs: https://coverage.nodejs.org/coverage-7123a00b03a90862/lib/internal/cluster/round_robin_handle.js.html#L85

Following that link will highlight line 85 of round_robin_handle.js which is currently not covered. Putting that at the end of the commit message would be great.

It's useful (or at least I think it is!) in PRs to increase code coverage to include a Refs: line showing what you are adding coverage for. For example:
Refs: https://coverage.nodejs.org/coverage-7123a00b03a90862/lib/internal/cluster/round_robin_handle.js.html#L85
Following that link will highlight line 85 of round_robin_handle.js which is currently not covered. Putting that at the end of the commit message would be great.

Perfect! Just added it to the PR, should I add it to the commit as well?

It's useful (or at least I think it is!) in PRs to increase code coverage to include a Refs: line showing what you are adding coverage for. For example:
Refs: https://coverage.nodejs.org/coverage-7123a00b03a90862/lib/internal/cluster/round_robin_handle.js.html#L85
Following that link will highlight line 85 of round_robin_handle.js which is currently not covered. Putting that at the end of the commit message would be great.

FWIW these are not permanent links -- we do not have the disk space to contain coverage data indefinitely.

It's useful (or at least I think it is!) in PRs to increase code coverage to include a Refs: line showing what you are adding coverage for. For example:
Refs: https://coverage.nodejs.org/coverage-7123a00b03a90862/lib/internal/cluster/round_robin_handle.js.html#L85
Following that link will highlight line 85 of round_robin_handle.js which is currently not covered. Putting that at the end of the commit message would be great.
Perfect! Just added it to the PR, should I add it to the commit as well?

I think adding them to the PR is enough. The commit will end up having a link to the PR anyway. And as Richard Lau pointed out, these links are not permanent, so it's probably fine to not have them in the commit message, contrary to my earlier suggestion.

Signed-off-by: Erick Wendel <erick.workspace@gmail.com>

nice work!

CI: https://ci.nodejs.org/job/node-test-pull-request/42315/

Hope this one isn't flaky like the other one

@benjamingr @Trott which one was that? I was talking with @ErickWendel today and I might have some idea of what the problem was/is

Looks like it was already fixed #41204 ?

Hm, that test is actually very very similar to this one. That also means that I’m wrong about having an idea about the cause of the flakiness :)

Landed in ba5b5ac

PR-URL: nodejs#41818 Refs: https://coverage.nodejs.org/coverage-7123a00b03a90862/lib/internal/worker.js.html#L412 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Added the dont-land-onv17.x label due to this tests failing on debug builds. See: #42072 (comment)

PR-URL: #41818 Refs: https://coverage.nodejs.org/coverage-7123a00b03a90862/lib/internal/worker.js.html#L412 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

I had to remove this commit from v16.x-staging as the test crashes there in debug mode. Probably a bug in V8.

ErickWendel added 2 commits February 1, 2022 18:06

worker: improve code coverage for assignEnvironmentData function

1b9e60b

worker: improve code coverage for getHeapSnapshot

0a3e3c7

nodejs-github-bot added needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels Feb 1, 2022

aduh95 reviewed Feb 1, 2022

View reviewed changes

Comment thread test/parallel/test-worker-heap-snapshot.js Outdated

worker: add mustCall to the event to ensure the assert will be called…

0bceab4

… in order

addaleax reviewed Feb 2, 2022

View reviewed changes

Comment thread lib/internal/worker.js Outdated

Comment thread test/parallel/test-worker-heap-snapshot.js Outdated

Comment thread test/parallel/test-worker-heap-snapshot.js Outdated

worker: add test for heapsnapshot as an integration test

3027e15

Signed-off-by: Erick Wendel <erick.workspace@gmail.com>

addaleax approved these changes Feb 2, 2022

View reviewed changes

Trott added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Feb 2, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 2, 2022

cjihrig approved these changes Feb 2, 2022

View reviewed changes

benjamingr approved these changes Feb 2, 2022

View reviewed changes

benjamingr reviewed Feb 2, 2022

View reviewed changes

jasnell approved these changes Feb 5, 2022

View reviewed changes

aduh95 merged commit ba5b5ac into nodejs:master Feb 11, 2022

bengl mentioned this pull request Feb 21, 2022

v17.6.0 release proposal #42072

Merged

bengl added the dont-land-on-v17.x label Feb 22, 2022

BethGriggs mentioned this pull request Apr 9, 2022

2022-04-19, Version 18.0.0 (Current) #42262

Merged

juanarbol mentioned this pull request May 31, 2022

2022-05-XX, Version 16.16.0 'Gallium' (LTS) #43268

Closed

targos added the dont-land-on-v16.x label Jul 31, 2022

targos mentioned this pull request Jul 31, 2022

[v16.x backport] node:test #43904

Closed

+              (async () => {
+                const worker = new Worker('setInterval(() => {}, 1000);', { eval: true });
+                await once(worker, 'online');
+                const stream = await worker.getHeapSnapshot();

worker: improve code coverage by ErickWendel · Pull Request #41818 · nodejs/node · GitHub

Uh oh!

Uh oh!

worker: improve code coverage - #41818

worker: improve code coverage#41818aduh95 merged 4 commits intonodejs:masternodejs/node:masterfrom ErickWendel:worker/improve-code-coverageErickWendel/node:worker/improve-code-coverageCopy head branch name to clipboard

Conversation

ErickWendel commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Trott commented Feb 1, 2022

Uh oh!

ErickWendel commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Feb 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Trott commented Feb 2, 2022

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 2, 2022

Uh oh!

benjamingr Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

benjamingr Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Feb 11, 2022

Uh oh!

bengl commented Feb 22, 2022

Uh oh!

targos commented Jul 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

worker: improve code coverage#41818
aduh95 merged 4 commits into
nodejs:masterfrom
ErickWendel:worker/improve-code-coverage

ErickWendel commented Feb 1, 2022 •
edited
Loading

ErickWendel commented Feb 1, 2022 •
edited
Loading