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

worker: improve code coverage #41818

Merged
merged 4 commits into from
Feb 11, 2022

Conversation

ErickWendel
Copy link
Member

@ErickWendel ErickWendel commented Feb 1, 2022

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

@nodejs-github-bot 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
@Trott
Copy link
Member

Trott commented Feb 1, 2022

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.

@ErickWendel
Copy link
Member Author

ErickWendel commented Feb 1, 2022

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?

@richardlau
Copy link
Member

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.

lib/internal/worker.js Outdated Show resolved Hide resolved
test/parallel/test-worker-heap-snapshot.js Outdated Show resolved Hide resolved
test/parallel/test-worker-heap-snapshot.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Feb 2, 2022

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>
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work!

@Trott 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 pending requests for changes, and a CI started. labels Feb 2, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 2, 2022
@nodejs-github-bot
Copy link
Collaborator

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope this one isn't flaky like the other one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it was already fixed #41204 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 11, 2022
@aduh95 aduh95 merged commit ba5b5ac into nodejs:master Feb 11, 2022
@aduh95
Copy link
Contributor

aduh95 commented Feb 11, 2022

Landed in ba5b5ac

bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
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>
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
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>
@bengl bengl mentioned this pull request Feb 21, 2022
@bengl
Copy link
Member

bengl commented Feb 22, 2022

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

juanarbol pushed a commit that referenced this pull request May 31, 2022
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>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
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>
targos pushed a commit that referenced this pull request Jul 11, 2022
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>
@targos
Copy link
Member

targos commented Jul 31, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.