Skip to content

test_runner: expose reporter for use in run api #47238

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
Merged
merged 7 commits into from
Mar 28, 2023
Merged

test_runner: expose reporter for use in run api #47238

merged 7 commits into from
Mar 28, 2023

Conversation

@atlowChemi
Copy link
Member

@atlowChemi atlowChemi commented Mar 23, 2023

Fixes: #47231

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 23, 2023

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Mar 23, 2023
MoLow
MoLow reviewed Mar 23, 2023
View reviewed changes
MoLow
MoLow reviewed Mar 23, 2023
View reviewed changes
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 24, 2023
anonrig
anonrig reviewed Mar 24, 2023
View reviewed changes
@cjihrig
Copy link
Contributor

cjihrig commented Mar 24, 2023

Isn't this going to reintroduce the same problem as #46061 (comment)

@ljharb
Copy link
Member

ljharb commented Mar 24, 2023

@cjihrig so would the addition of any core modules; see #46061 (comment) and #46061 (comment)

benjamingr
benjamingr approved these changes Mar 25, 2023
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Code lgtm. All anonrig's suggestions look good to me too.

@MoLow
Copy link
Member

MoLow commented Mar 25, 2023

Isn't this going to reintroduce the same problem as #46061 (comment)

yeah but as @ljharb mentioned this is desired and will require fixing is-core-module

MoLow
MoLow reviewed Mar 25, 2023
View reviewed changes
MoLow
MoLow reviewed Mar 25, 2023
View reviewed changes
ljharb
ljharb requested changes Mar 25, 2023
View reviewed changes
lib/test/reporters.js
let spec;
let tap;

ObjectDefineProperties(module.exports, {
Copy link
Member

@ljharb ljharb Mar 25, 2023

Choose a reason for hiding this comment

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

are we sure that cjs-module-lexer will be able to pick up this pattern as named exports?

Copy link
Member

@MoLow MoLow Mar 26, 2023

Choose a reason for hiding this comment

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

it is a pretty common pattern in node's built-in modules, so it probably does:

node/lib/crypto.js

Line 340 in 9dbb162

ObjectDefineProperties(module.exports, {

node/lib/buffer.js

Line 1392 in 9dbb162

ObjectDefineProperties(module.exports, {

node/lib/dns.js

Line 345 in 9dbb162

ObjectDefineProperties(module.exports, {

node/lib/os.js

Line 399 in 9dbb162

ObjectDefineProperties(module.exports, {

node/lib/zlib.js

Line 926 in 9dbb162

ObjectDefineProperties(module.exports, {

Copy link
Contributor

@aduh95 aduh95 Mar 26, 2023

Choose a reason for hiding this comment

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

It probably doesn’t but we don’t use cjs-module-lexer for core modules, because core modules are not CJS (or at least, not regular CJS) and the use of named imports for core modules predates the introduction of cjs-module-lexer.

@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 26, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 26, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 26, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 26, 2023

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

@MoLow MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 26, 2023
@MoLow MoLow added the test_runner Issues and PRs related to the test runner subsystem. label Mar 26, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 28, 2023

Landed in 29d7aec

@github-actions github-actions bot mentioned this pull request Mar 30, 2023
Open
18 tasks
This was referenced Apr 3, 2023
Merged
Merged
@RafaelGSS RafaelGSS added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 10, 2023
@atlowChemi atlowChemi deleted the expose-test-stream-reportes branch April 13, 2023 20:48
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig left review comments

@aduh95 aduh95 aduh95 left review comments

@ljharb ljharb ljharb requested changes

@benjamingr benjamingr benjamingr approved these changes

@MoLow MoLow MoLow 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. semver-minor PRs that contain new features and should be released in the next minor version. test_runner Issues and PRs related to the test runner subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Test runner run function crashing

9 participants

@atlowChemi @nodejs-github-bot @cjihrig @ljharb @MoLow @benjamingr @anonrig @aduh95 @RafaelGSS