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: refactor test-abortcontroller to use node:test #54574

Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 26, 2024

Starting the long process of refactoring our own tests to use the node:test module and mocks.

Also, splits the test that depends on internal API from the rest of the tests to make it easier to differentiate.

Starting the long process of refactoring our own tests to use
the node:test module and mocks.
@jasnell jasnell requested a review from anonrig August 26, 2024 20:49
@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 Aug 26, 2024
@jasnell jasnell requested a review from mcollina August 26, 2024 20:49
@@ -1,7 +1,7 @@
// Flags: --no-warnings --expose-gc --expose-internals
// Flags: --expose-gc
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add an eslint rule to avoid re-adding expose-internals to files that we avoid.

@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2024
@nodejs-github-bot

This comment was marked as outdated.

This comment was marked as outdated.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 27, 2024

Starting the long process of refactoring our own tests to use the node:test module

Not that I mind, but I'm curious what the motivation is for this? I'm assuming something related to Workers.

@jasnell
Copy link
Member Author

jasnell commented Aug 27, 2024

Not that I mind, but I'm curious what the motivation is for this? I'm assuming something related to Workers.

Key motivation is make the tests more structured and easier to decompose... and yes, it's largely for other runtimes that want to be able to run/port the tests more easily in order to better verify Node.js compatibility. But it's also just good to have improved structure in our own tests just for us.

@lpinca
Copy link
Member

lpinca commented Aug 27, 2024

Key motivation is make the tests more structured and easier to decompose... and yes, it's largely for other runtimes that want to be able to run/port the tests more easily in order to better verify Node.js compatibility

To be honest, I think it goes in the opposite direction. Using BDD specific syntax does not help.

@jasnell
Copy link
Member Author

jasnell commented Aug 27, 2024

... Using BDD specific syntax does not help.

As someone who has been working to port many of these tests to another runtime adding this kind of structure does help significantly. The current unstructured tests mix a number of public API, internal API, and test harness code in ways that make it difficult and time consuming to decompose or, often, to even know what exactly is being tested. Sure, the organization can be further improved but introducing some structure and organization is objectively better than having none, which is what we have now.

@nodejs-github-bot

This comment was marked as outdated.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 27, 2024

The current unstructured tests mix a number of public API, internal API, and test harness code in ways that make it difficult and time consuming to decompose or, often, to even know what exactly is being tested.

As someone who has also ported many Node tests to another runtime, I can confirm that was a pain point for me at the time.

@lpinca
Copy link
Member

lpinca commented Aug 27, 2024

The current unstructured tests mix a number of public API, internal API, and test harness code in ways that make it difficult and time consuming to decompose or, often, to even know what exactly is being tested

Using node:test does not change that, it only add an additional module to the mix and a specific non portable test syntax. I think there is nothing better than a single file per test with no dependencies other than the strictly necessary ones. I said it elsewhere but I'll say it again. I have always loved the way tests were made and run in Node.js: a single script with no bullshit just like the minimal test cases we maintainers ask for bug reports.

@jasnell
Copy link
Member Author

jasnell commented Aug 27, 2024

... Using node:test does not change that,

Not sure we'll agree on this point. Decomposing the tests into more structured units; going through the tests and relying, as much as possible, on public API surface (for instance, replacing common.mustCall(...) with mock.fn() checks); and having cleaner separation between things that are testing public API vs internal API helps significantly in porting these tests to other environments to ensure compatibility. Sure, the syntax isn't perfect but this absolutely helps the process along.

@lpinca
Copy link
Member

lpinca commented Aug 27, 2024

replacing common.mustCall(...) with mock.fn() checks

Yes, I disagree. The common module is still required, so we actually end up duplicating/adding more code.

@jasnell jasnell added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Aug 28, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 28, 2024

jasnell added a commit that referenced this pull request Aug 29, 2024
Starting the long process of refactoring our own tests to use
the node:test module and mocks.

PR-URL: #54574
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Aug 29, 2024

Landed in 39215e1

@jasnell jasnell closed this Aug 29, 2024
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
Starting the long process of refactoring our own tests to use
the node:test module and mocks.

PR-URL: #54574
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
Starting the long process of refactoring our own tests to use
the node:test module and mocks.

PR-URL: #54574
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
Starting the long process of refactoring our own tests to use
the node:test module and mocks.

PR-URL: #54574
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Aug 30, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 22, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
Starting the long process of refactoring our own tests to use
the node:test module and mocks.

PR-URL: nodejs#54574
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. 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