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.

Sign up for GitHub

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

Jump to bottom

worker: improve code coverage #41818

Merged
merged 4 commits into from Feb 11, 2022
Merged

worker: improve code coverage #41818

merged 4 commits into from Feb 11, 2022

Conversation

ErickWendel
Copy link
Member

@ErickWendel ErickWendel commented Feb 1, 2022 •
edited

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 •
edited

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.

addaleax
addaleax reviewed Feb 2, 2022
View reviewed changes
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.

@ErickWendel
worker: add test for heapsnapshot as an integration test
3027e15 
Signed-off-by: Erick Wendel <erick.workspace@gmail.com>
addaleax
addaleax approved these changes Feb 2, 2022
View reviewed changes
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

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

benjamingr
benjamingr reviewed Feb 2, 2022
View reviewed changes
test/parallel/test-worker-heap-snapshot.js
(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
@ErickWendel @bengl
test: improve worker_threads coverage
626577e 
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
@ErickWendel @bengl
test: improve worker_threads coverage
8a9f072 
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
Merged
@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)

@BethGriggs BethGriggs mentioned this pull request Apr 9, 2022
Merged
juanarbol pushed a commit that referenced this pull request May 31, 2022
@ErickWendel @juanarbol
test: improve worker_threads coverage
b96cfde 
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>
@juanarbol juanarbol mentioned this pull request May 31, 2022
Closed
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
@ErickWendel @danielleadams
test: improve worker_threads coverage
528c659 
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
@ErickWendel @targos
test: improve worker_threads coverage
816dadb 
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.

@targos targos mentioned this pull request Jul 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@benjamingr benjamingr benjamingr approved these changes

@aduh95 aduh95 aduh95 left review comments

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet