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

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

Merged
merged 5 commits into from
Dec 24, 2023

Conversation

pulkit-30
Copy link
Contributor

@pulkit-30 pulkit-30 commented Nov 30, 2023

Fix #48809

pass testStream to setup instead testRoot.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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 pulkit-30 marked this pull request as ready for review December 3, 2023 12:36
Copy link
Member

@rluvaton rluvaton left a comment

Choose a reason for hiding this comment

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

Can you please add a test

@pulkit-30 pulkit-30 requested a review from rluvaton December 4, 2023 04:38
Copy link
Member

@rluvaton rluvaton left a comment

Choose a reason for hiding this comment

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

Well done! thanks

@rluvaton rluvaton added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@pulkit-30 pulkit-30 requested a review from MoLow December 4, 2023 15:29
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

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
image

but on this branch the colors are discarded:
image

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

@pulkit-30
Copy link
Contributor Author

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

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

@pulkit-30
Copy link
Contributor Author

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

@pulkit-30 pulkit-30 marked this pull request as draft December 6, 2023 01:51
@pulkit-30 pulkit-30 marked this pull request as ready for review December 6, 2023 17:01
@pulkit-30 pulkit-30 requested a review from MoLow December 11, 2023 15:49
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

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

@pulkit-30 pulkit-30 requested a review from MoLow December 15, 2023 16:50
@pulkit-30
Copy link
Contributor Author

@MoLow PTAL.

@MoLow 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 23, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit dad666a into nodejs:main Dec 24, 2023
56 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in dad666a

@MoLow
Copy link
Member

MoLow commented Dec 24, 2023

@pulkit-30 thanks for the hard work 🎉

@pulkit-30
Copy link
Contributor Author

@pulkit-30 thanks for the hard work 🎉

Thanks a ton! It means a lot.

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
PR-URL: #50982
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
@koshic
Copy link

koshic commented Jan 16, 2024

@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.

@MoLow
Copy link
Member

MoLow commented Jan 16, 2024

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

richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #50982
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@richardlau richardlau mentioned this pull request Mar 25, 2024
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. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_runner: test object is incorrectly passed to setup()
6 participants