-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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: todo, only, skip are expected to return a Promise #48555
Conversation
Review requested:
|
The implementation looks ok, can you add a test, or modify existing tests? |
I added a test, I'm not really sure if the structure is correct |
@shockerqt FYI - The first commit has to be prefixed with node/doc/contributing/pull-requests.md Lines 158 to 175 in 821c487
|
I don't know if there is a way to edit the first commit, I read the guidelines too late 😔 |
There might be another solution I am not aware of, but you could |
`test.todo`, `test.only` and `test.skip` are expected to return the same as `test`. This commit corrects the inconsistent behavior of these shorthands. Fixes: nodejs#48557
Added test to check that the return type of the `todo`, `only` and `skip` shorthands are consistent with the return type of `test`.
Thanks! I managed to fix the message of the first commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Commit Queue failed- Loading data for nodejs/node/pull/48555 ✔ Done loading data for nodejs/node/pull/48555 ----------------------------------- PR info ------------------------------------ Title test_runner: todo, only, skip are expected to return a Promise (#48555) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch shockerqt:main -> nodejs:main Labels author ready, needs-ci, test_runner Commits 3 - test_runner: fixed `test` shorthands return type - test_runner: added tests for test shorthands - test_runner: change tests to use `describe/it` Committers 1 - shocker PR-URL: https://github.com/nodejs/node/pull/48555 Reviewed-By: Moshe Atlow ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/48555 Reviewed-By: Moshe Atlow -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 26 Jun 2023 04:10:55 GMT ✔ Approvals: 1 ✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/48555#pullrequestreview-1500033494 ✘ This PR needs to wait 20 more hours to land (or 0 hours if there is one more approval) ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-07-02T06:53:42Z: https://ci.nodejs.org/job/node-test-pull-request/52583/ - Querying data for job/node-test-pull-request/52583/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/5435457602 |
Commit Queue failed- Loading data for nodejs/node/pull/48555 ✔ Done loading data for nodejs/node/pull/48555 ----------------------------------- PR info ------------------------------------ Title test_runner: todo, only, skip are expected to return a Promise (#48555) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch shockerqt:main -> nodejs:main Labels author ready, needs-ci, test_runner Commits 3 - test_runner: fixed `test` shorthands return type - test_runner: added tests for test shorthands - test_runner: change tests to use `describe/it` Committers 1 - shocker PR-URL: https://github.com/nodejs/node/pull/48555 Reviewed-By: Moshe Atlow ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/48555 Reviewed-By: Moshe Atlow -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 26 Jun 2023 04:10:55 GMT ✔ Approvals: 1 ✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/48555#pullrequestreview-1500033494 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-07-02T07:59:05Z: https://ci.nodejs.org/job/node-test-pull-request/52583/ - Querying data for job/node-test-pull-request/52583/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 48555 From https://github.com/nodejs/node * branch refs/pull/48555/merge -> FETCH_HEAD ✔ Fetched commits as 1aabfa8732fb..cf59fa00fb6c -------------------------------------------------------------------------------- [main b350b9bf63] test_runner: fixed `test` shorthands return type Author: Shocker <43253032+shockerqt@users.noreply.github.com> Date: Sun Jun 25 23:47:06 2023 -0400 1 file changed, 1 insertion(+), 3 deletions(-) [main 8dd5fc58ea] test_runner: added tests for test shorthands Author: shocker Date: Mon Jun 26 07:20:14 2023 -0400 1 file changed, 34 insertions(+) create mode 100644 test/parallel/test-runner-typechecking.js [main 1aa8125728] test_runner: change tests to use `describe/it` Author: shocker Date: Mon Jun 26 21:07:55 2023 -0400 1 file changed, 26 insertions(+), 24 deletions(-) ✔ Patches applied There are 3 commits in the PR. Attempting autorebase. Rebasing (2/6)https://github.com/nodejs/node/actions/runs/5442365030 |
Landed in d312bd9 |
`test.todo`, `test.only` and `test.skip` are expected to return the same as `test`. This commit corrects the inconsistent behavior of these shorthands. Fixes: nodejs#48557 PR-URL: nodejs#48555 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
`test.todo`, `test.only` and `test.skip` are expected to return the same as `test`. This commit corrects the inconsistent behavior of these shorthands. Fixes: nodejs#48557 PR-URL: nodejs#48555 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
todo
,only
andskip
are shorthands fortest
, so all of them should return the same type.Currently
todo()
returnsundefined
whiletest({ todo: true })
returns aPromise
that resolves toundefined
.