Skip to content

Comments

stream: update readable.map test to use node:test#47900

Closed
rluvaton wants to merge 1 commit intonodejs:mainfrom
rluvaton:convert-test-to-node-test
Closed

stream: update readable.map test to use node:test#47900
rluvaton wants to merge 1 commit intonodejs:mainfrom
rluvaton:convert-test-to-node-test

Conversation

@rluvaton
Copy link
Member

@rluvaton rluvaton commented May 6, 2023

this is part of a bigger question: should node use its own testing?

if so, this is the first step

If speaking from DX's point of view I think it should be as the tooling is only going to get better, and you going to have an easier way of writing and executing tests IMO

@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 May 6, 2023
Copy link
Member

@debadree25 debadree25 left a comment

Choose a reason for hiding this comment

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

Would it conflict with the python test runner we use? If we do this also we would have to migrate all tests no? some test in node:test some tests in legacy then

@cjihrig
Copy link
Contributor

cjihrig commented May 6, 2023

this is part of a bigger question: should node use its own testing?

Some people have started writing new tests using node:test. Other people prefer not to because writing a plain JS file is pretty easy. This was discussed a bit in #44288

Would it conflict with the python test runner we use?

There is already some mixture of the two test runners that seems to work well enough.

If we do this also we would have to migrate all tests no?

No. This would be a massive amount of churn that doesn't accomplish much IMO and is likely to introduce backporting conflicts. Additionally, the state of the test runner is different on the various release lines which could also introduce friction.

@MoLow
Copy link
Member

MoLow commented May 7, 2023

Would it conflict with the python test runner we use? If we do this also we would have to migrate all tests no?

no, it won't. the python test runner really only cares about the process exit code

@MoLow
Copy link
Member

MoLow commented May 7, 2023

LGTM, but the commit message should start with test:, not stream:

@debadree25
Copy link
Member

Ah thanks for explaining! if it doesnt then i think this is good then!

@aduh95
Copy link
Contributor

aduh95 commented May 7, 2023

Rewriting existing tests is high risk, low reward. I’m generally -0 on these kind of proposals, and also the test runner node --test works fine with plain JS files, migrating to using node:test in the test files is not a requirement for moving away from the Python runner (if that was the goal).

@rluvaton
Copy link
Member Author

Will close this than πŸ˜„

@rluvaton rluvaton closed this May 14, 2023
@rluvaton rluvaton deleted the convert-test-to-node-test branch May 14, 2023 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants