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

doc: update todo test behaviour #51427

Closed
wants to merge 1 commit into from

Conversation

pulkit-30
Copy link
Contributor

@pulkit-30 pulkit-30 commented Jan 10, 2024

@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 Jan 10, 2024
@aduh95
Copy link
Contributor

aduh95 commented Jan 10, 2024

I think the idea is to still get the test result, otherwise there are no distinction between skip and todo and we might as keep only one 🤔

@marco-ippolito
Copy link
Member

marco-ippolito commented Jan 11, 2024

Todo test should fail but not exit with code 1, not affecting the result. Making it a noop makes it just an alias of skip.
I think there is a bug that makes todo test exit

@pulkit-30
Copy link
Contributor Author

I think the idea is to still get the test result, otherwise there are no distinction between skip and todo and we might as keep only one 🤔

I think running a todo test doesn't make any sense as it will fails coz it's not fully implemented or maybe planned to write it later on.

One thing that we can do to make a distinction between skip and todo as to restrict user to not pass test fn when test is marked as todo (similar to jest-todo).

@pulkit-30
Copy link
Contributor Author

Todo test should fail but not exit with code 1, not affecting the result.

+1

@pulkit-30
Copy link
Contributor Author

Todo test should fail but not exit with code 1, not affecting the result. Making it a noop makes it just an alias of skip. I think there is a bug that makes todo test exit

I found that todo test runs and does not terminate execution of the test function. also if it fails it doesnot mark exitCode as 1.
I have documented this behavior coz i didn't find anything about this.

@pulkit-30 pulkit-30 changed the title test_runner: pass noop fn when test marked as todo doc: doc: update todo test behaviour Jan 11, 2024
@pulkit-30 pulkit-30 changed the title doc: doc: update todo test behaviour doc: update todo test behaviour Jan 11, 2024
Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

I'm not sure the wording is right.
AFAIK the failure of a test does not terminate the execution it makes exit code 1.
Saying that todo does not terminate it imples that failure do which I dont think is correct

@@ -1250,6 +1250,10 @@ However, the return value from subtests should be used to prevent the parent
test from finishing first and cancelling the subtest
as shown in the following example.

**Note:** Running a `todo` test does not terminate the execution of the test
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**Note:** Running a `todo` test does not terminate the execution of the test
Running a `todo` test does not terminate the execution of the test

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.

Should this be superseded by #52204?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 28, 2024

Closing per the previous comment.

@cjihrig cjihrig closed this Mar 28, 2024
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_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants