Skip to content

test_runner: support coverage of unnamed functions - #47652

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
cjihrig:anon-fns
Apr 23, 2023
Merged

test_runner: support coverage of unnamed functions#47652
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
cjihrig:anon-fns

Conversation

@cjihrig

Copy link
Copy Markdown
Contributor

This commit updates the code coverage logic to skip the first function in a file (which does not correspond to an actual function) instead of skipping all functions without a name.

This commit updates the code coverage logic to skip the first
function in a file (which does not correspond to an actual
function) instead of skipping all functions without a name.
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Apr 20, 2023
}

if (functionName.length > 0 && ranges.length > 0) {
if (j > 0 && ranges.length > 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this also correct in the case of ESM?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i have the same question; CJS is wrapped in a function but i don't expect ESM to be

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes. I thought maybe it was a CJS specific thing, but it's not.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

 {
"scriptId": "422",
"url": "file:///Users/cjihrig/iojs/node/test.mjs",
"functions": [
{
"functionName": "",
"ranges": [
{
"startOffset": 0,
"endOffset": 4462,
"count": 1
}
],
"isBlockCoverage": true
},
{
"functionName": "chickenSandwich",
"ranges": [
{
"startOffset": 49,
"endOffset": 80,
"count": 1
}
],
"isBlockCoverage": true
}
]
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

weird, is that something added by v8, or by node's esm implementation?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That part is unclear to me.

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@MoLowMoLow 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 Apr 22, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@MoLowMoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 23, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 23, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 53e3c0e into nodejs:mainApr 23, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 53e3c0e

targos pushed a commit that referenced this pull request May 2, 2023
This commit updates the code coverage logic to skip the first
function in a file (which does not correspond to an actual
function) instead of skipping all functions without a name.
PR-URL: #47652
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targostargos mentioned this pull request May 2, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
This commit updates the code coverage logic to skip the first
function in a file (which does not correspond to an actual
function) instead of skipping all functions without a name.
PR-URL: #47652
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
This commit updates the code coverage logic to skip the first
function in a file (which does not correspond to an actual
function) instead of skipping all functions without a name.
PR-URL: nodejs#47652
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.test_runnerIssues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@cjihrig@nodejs-github-bot@ljharb@mcollina@benjamingr@MoLow