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

Add test.todo to match it.todo in node:test #47897

Closed
TomasHubelbauer opened this issue May 6, 2023 · 5 comments · Fixed by #47909
Closed

Add test.todo to match it.todo in node:test #47897

TomasHubelbauer opened this issue May 6, 2023 · 5 comments · Fixed by #47909
Labels
feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.

Comments

@TomasHubelbauer
Copy link

What is the problem this feature will solve?

In the Node test runner (https://nodejs.org/api/test.html#test-runner) we have describe.todo and it.todo to designate suites/tests which do not exist yet but we're planning on adding. The describe > it syntax is one option of declaring tests, though, there is also just test in the API. For those of us who prefer test over describe, there is no shorthand test.todo, only the test method todo option: https://nodejs.org/api/test.html#testname-options-fn

What is the feature you are proposing to solve the problem?

Add test.todo similarly to how it.todo currently exists:

    namespace it {
        // Shorthand for skipping a test, same as `it([name], { skip: true }[, fn])`.
        function skip(name?: string, options?: TestOptions, fn?: ItFn): void;
        function skip(name?: string, fn?: ItFn): void;
        function skip(options?: TestOptions, fn?: ItFn): void;
        function skip(fn?: ItFn): void;

        // Shorthand for marking a test as `TODO`, same as `it([name], { todo: true }[, fn])`.
        function todo(name?: string, options?: TestOptions, fn?: ItFn): void;
        function todo(name?: string, fn?: ItFn): void;
        function todo(options?: TestOptions, fn?: ItFn): void;
        function todo(fn?: ItFn): void;
    }

What alternatives have you considered?

Not doing it - we do have the test todo option so a shorthand is a QOL/DX improvement not a blocker of anything. I don't see the reason for why it should have it and test not though. Seems like parity on these would not hurt anything.

@TomasHubelbauer TomasHubelbauer added the feature request Issues that request new features to be added to Node.js. label May 6, 2023
@VoltrexKeyva VoltrexKeyva added the test_runner Issues and PRs related to the test runner subsystem. label May 7, 2023
@MoLow
Copy link
Member

MoLow commented May 7, 2023

CC @cjihrig - this does not exist mainly due to your objection so the API would remain simple. do you still object to this change?

@cjihrig
Copy link
Contributor

cjihrig commented May 7, 2023

I do still want the API to remain simple - even small changes add up. I also think this API exactly duplicates existing functionality so it doesn't really buy us anything.

That said, it keeps coming up and I'm tired of repeating myself so I won't block it.

@TomasHubelbauer
Copy link
Author

I honestly just find it weird to have to import it for todo when I'm using test for everything else. Simple API is a great goal of course, but it seems like this parity between test and it would reduce surprises and align with user's expectations so as a result arguably it being the would be "simpler" in some sense. Just my 0.02 USD as a user.

@cjihrig
Copy link
Contributor

cjihrig commented May 7, 2023

I didn't want to add that API for it() either.

@MoLow MoLow added the good first issue Issues that are suitable for first-time contributors. label May 7, 2023
nodejs-github-bot pushed a commit that referenced this issue May 15, 2023
PR-URL: #47909
Fixes: #47897
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit that referenced this issue May 15, 2023
PR-URL: #47909
Fixes: #47897
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
danielleadams pushed a commit that referenced this issue Jul 6, 2023
PR-URL: #47909
Fixes: #47897
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
MoLow pushed a commit to MoLow/node that referenced this issue Jul 6, 2023
PR-URL: nodejs#47909
Fixes: nodejs#47897
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@ForbiddenEra
Copy link

Hopefully not too OT but .. should we have the same for before and after, IIRC I was only able to use beforeEach and afterEach with test ..?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants