test_runner: fixed test object is incorrectly passed to setup() by pulkit-30 · Pull Request #50982 · nodejs/node · GitHub

pass testStream to setup instead testRoot.

Review requested:

@nodejs/test_runner

Can you please add a test

Well done! thanks

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

this PR causes a regression:
running a simple test file:

const test = require("node:test");
test("a", () => console.log(1));

on main console is printed with colors

but on this branch the colors are discarded:

I am working on fixing the test that should have caught this

this PR causes a regression:
running a simple test file:

I was also suspecting something, but surprised by the tests 😅

converting this PR to draft. ref: #51064, still working on it 🚀

createWriteStream is called multiple times for each reporter destination, and in some cases the writeStream is never being closed

@MoLow PTAL.

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

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

Landed in dad666a

@pulkit-30 thanks for the hard work 🎉

@pulkit-30 thanks for the hard work 🎉

Thanks a ton! It means a lot.

PR-URL: #50982 Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

@MoLow do you plan to backport it to 20.x? Documentation is still incorrect - https://nodejs.org/docs/latest-v20.x/api/test.html#runoptions.

not every PR needs a manual backport. lets wait for @nodejs/releasers to release a new v20 version and request a backport if needed

PR-URL: #50982 Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Nov 30, 2023

pulkit-30 commented Nov 30, 2023

View reviewed changes

Comment thread lib/internal/test_runner/utils.js Outdated

pulkit-30 marked this pull request as ready for review December 3, 2023 12:36

rluvaton reviewed Dec 3, 2023

View reviewed changes

pulkit-30 requested a review from rluvaton December 4, 2023 04:38

rluvaton reviewed Dec 4, 2023

View reviewed changes

Comment thread test/parallel/test-runner-run.mjs Outdated

rluvaton reviewed Dec 4, 2023

View reviewed changes

Comment thread test/parallel/test-runner-run.mjs Outdated

pulkit-30 force-pushed the fix-48809 branch from 4086899 to 7513467 Compare December 4, 2023 11:08

rluvaton reviewed Dec 4, 2023

View reviewed changes

Comment thread test/parallel/test-runner-run.mjs Outdated

rluvaton approved these changes Dec 4, 2023

View reviewed changes

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

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023

pulkit-30 requested a review from MoLow December 4, 2023 15:29

MoLow requested changes Dec 5, 2023

View reviewed changes

MoLow mentioned this pull request Dec 5, 2023

test: fix test runner colored output test #51064

Merged

pulkit-30 marked this pull request as draft December 6, 2023 01:51

pulkit-30 force-pushed the fix-48809 branch from 3c8c335 to 79d22e5 Compare December 6, 2023 17:01

pulkit-30 marked this pull request as ready for review December 6, 2023 17:01

pulkit-30 requested a review from MoLow December 11, 2023 15:49

pulkit-30 force-pushed the fix-48809 branch from 79d22e5 to 6a3ebf7 Compare December 11, 2023 15:51

MoLow reviewed Dec 11, 2023

View reviewed changes

Comment thread test/parallel/test-runner-run.mjs Outdated

MoLow reviewed Dec 11, 2023

View reviewed changes

Comment thread lib/internal/test_runner/utils.js Outdated

MoLow requested changes Dec 11, 2023

View reviewed changes

test_runner: fixed test object is incorrectly passed to setup()

e4eb26b

fix suggestions

175af14

pulkit-30 force-pushed the fix-48809 branch from 6a3ebf7 to 175af14 Compare December 12, 2023 10:57

pulkit-30 added 2 commits December 12, 2023 16:30

fix lint

775ea08

call colorizeTestFil fn before abort

9f64e75

MoLow reviewed Dec 12, 2023

View reviewed changes

Comment thread lib/internal/test_runner/utils.js Outdated

MoLow reviewed Dec 12, 2023

View reviewed changes

Comment thread lib/internal/test_runner/harness.js Outdated

MoLow reviewed Dec 12, 2023

View reviewed changes

Comment thread lib/internal/test_runner/runner.js Outdated

fix suggestions

ca75b72

pulkit-30 requested a review from MoLow December 15, 2023 16:50

MoLow approved these changes Dec 23, 2023

View reviewed changes

MoLow added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 23, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 23, 2023

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2023

nodejs-github-bot merged commit dad666a into nodejs:main Dec 24, 2023

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024

test_runner: fixed test object is incorrectly passed to setup()

979cebc

PR-URL: #50982 Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

RafaelGSS mentioned this pull request Jan 2, 2024

v21.6.0 proposal #51342

Merged

richardlau pushed a commit that referenced this pull request Mar 25, 2024

test_runner: fixed test object is incorrectly passed to setup()

34ecd1e

PR-URL: #50982 Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

test_runner: fixed test object is incorrectly passed to setup() by pulkit-30 · Pull Request #50982 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: fixed test object is incorrectly passed to setup() - #50982

test_runner: fixed test object is incorrectly passed to setup()#50982nodejs-github-bot merged 5 commits intonodejs:mainnodejs/node:mainfrom pulkit-30:fix-48809pulkit-30/node:fix-48809Copy head branch name to clipboard

Conversation

pulkit-30 commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 30, 2023

Uh oh!

Uh oh!

rluvaton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rluvaton left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 4, 2023

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

pulkit-30 commented Dec 6, 2023

Uh oh!

pulkit-30 commented Dec 6, 2023

Uh oh!

Uh oh!

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pulkit-30 commented Dec 23, 2023

Uh oh!

nodejs-github-bot commented Dec 23, 2023

Uh oh!

nodejs-github-bot commented Dec 24, 2023

Uh oh!

nodejs-github-bot commented Dec 24, 2023

Uh oh!

MoLow commented Dec 24, 2023

Uh oh!

pulkit-30 commented Dec 24, 2023

Uh oh!

koshic commented Jan 16, 2024

Uh oh!

MoLow commented Jan 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test_runner: fixed test object is incorrectly passed to setup()#50982
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
pulkit-30:fix-48809

pulkit-30 commented Nov 30, 2023 •
edited
Loading