test_runner: todo, only, skip are expected to return a Promise #48555

shockerqt · 2023-06-26T04:10:55Z

todo, only and skip are shorthands for test, so all of them should return the same type.

Currently todo() returns undefined while test({ todo: true }) returns a Promise that resolves to undefined.

nodejs-github-bot · 2023-06-26T04:10:59Z

Review requested:

@nodejs/test_runner

MoLow · 2023-06-26T05:10:59Z

The implementation looks ok, can you add a test, or modify existing tests?

shockerqt · 2023-06-26T11:29:29Z

I added a test, I'm not really sure if the structure is correct

test/parallel/test-runner-typechecking.js

atlowChemi · 2023-06-26T20:03:09Z

@shockerqt FYI - The first commit has to be prefixed with test_runner: 🙏🏽

node/doc/contributing/pull-requests.md

Lines 158 to 175 in 821c487

    
           #### Commit message guidelines 
        
           A good commit message should describe what changed and why. 
        
           1. The first line should: 
        
              * contain a short description of the change (preferably 50 characters or 
        
                less, and no more than 72 characters) 
        
              * be entirely in lowercase with the exception of proper nouns, acronyms, and 
        
                the words that refer to code, like function/variable names 
        
              * be prefixed with the name of the changed [subsystem](#appendix-subsystems) 
        
                and start with an imperative verb. Check the output of `git log --oneline 
        
                files/you/changed` to find out what subsystems your changes touch. 
        
              Examples: 
        
              * `net: add localAddress and localPort to Socket` 
        
              * `src: fix typos in async_wrap.h`

shockerqt · 2023-06-26T22:51:05Z

I don't know if there is a way to edit the first commit, I read the guidelines too late 😔

atlowChemi · 2023-06-27T04:05:07Z

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 rebase, and reword the first commit:
https://git-scm.com/docs/git-rebase
https://stackoverflow.com/a/30194975
https://coderwall.com/p/_thg9a/reword-a-git-commit-message

`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`.

shockerqt · 2023-06-27T05:42:42Z

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 rebase, and reword the first commit: https://git-scm.com/docs/git-rebase https://stackoverflow.com/a/30194975 https://coderwall.com/p/_thg9a/reword-a-git-commit-message

Thanks! I managed to fix the message of the first commit

atlowChemi

LGTM

nodejs-github-bot · 2023-06-27T09:18:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52506/

nodejs-github-bot · 2023-07-01T18:35:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52577/

nodejs-github-bot · 2023-07-02T07:59:05Z

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/.ncu

https://github.com/nodejs/node/actions/runs/5435457602

nodejs-github-bot · 2023-07-03T08:38:31Z

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 [email protected]>
 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)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

test_runner: fixed test shorthands return type
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: #48557

PR-URL: #48555

Reviewed-By: Moshe Atlow [email protected]
[detached HEAD 34e98d1da4] test_runner: fixed test shorthands return type

Author: Shocker 43253032 [email protected]

Date: Sun Jun 25 23:47:06 2023 -0400

1 file changed, 1 insertion( ), 3 deletions(-)

Rebasing (3/6)

Rebasing (4/6)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

test_runner: added tests for test shorthands
Added test to check that the return type of the todo, only and skip shorthands

are consistent with the return type of test.
PR-URL: #48555

Reviewed-By: Moshe Atlow [email protected]
[detached HEAD 6de8d8de15] test_runner: added tests for test shorthands

Author: shocker [email protected]

Date: Mon Jun 26 07:20:14 2023 -0400

1 file changed, 34 insertions( )

create mode 100644 test/parallel/test-runner-typechecking.js

Rebasing (5/6)

Rebasing (6/6)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

test_runner: change tests to use describe/it
PR-URL: #48555

Reviewed-By: Moshe Atlow [email protected]
[detached HEAD 4d4c40489d] test_runner: change tests to use describe/it

Author: shocker [email protected]

Date: Mon Jun 26 21:07:55 2023 -0400

1 file changed, 26 insertions( ), 24 deletions(-)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/5442365030

nodejs-github-bot · 2023-07-03T09:04:13Z

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: #48557 PR-URL: #48555 Reviewed-By: Moshe Atlow <[email protected]>

`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 <[email protected]>

`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: #48557 PR-URL: #48555 Reviewed-By: Moshe Atlow <[email protected]>

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 Jun 26, 2023

MoLow linked an issue Jun 26, 2023 that may be closed by this pull request

[node:test] Test shorthands return undefined #48557

Closed

shockerqt changed the title ~~test: todo, only, skip are expected to return a Promise~~ test_runner: todo, only, skip are expected to return a Promise Jun 26, 2023

atlowChemi reviewed Jun 26, 2023

View reviewed changes

test/parallel/test-runner-typechecking.js Outdated Show resolved Hide resolved

shockerqt and others added 3 commits June 27, 2023 01:24

test_runner: fixed test shorthands return type

6068403

`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

test_runner: added tests for test shorthands

6ce6729

Added test to check that the return type of the `todo`, `only` and `skip` shorthands are consistent with the return type of `test`.

test_runner: change tests to use describe/it

cf59fa0

shockerqt force-pushed the main branch from 84c49dc to cf59fa0 Compare June 27, 2023 05:39

atlowChemi approved these changes Jun 27, 2023

View reviewed changes

MoLow approved these changes Jun 27, 2023

View reviewed changes

atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2023

atlowChemi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 27, 2023

This was referenced Jun 28, 2023

CI Reliability 2023-06-28 nodejs/reliability#601

Open

CI Reliability 2023-06-29 nodejs/reliability#602

Open

shockerqt mentioned this pull request Jun 29, 2023

[node:test] Update types DefinitelyTyped/DefinitelyTyped#65871

Merged

7 tasks

This was referenced Jun 30, 2023

CI Reliability 2023-06-30 nodejs/reliability#603

Open

CI Reliability 2023-07-01 nodejs/reliability#604

Open

atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 1, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 1, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 2, 2023

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jul 2, 2023

github-actions bot mentioned this pull request Jul 3, 2023

CI Reliability 2023-07-03 nodejs/reliability#606

Open

24 tasks

MoLow added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 3, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 3, 2023

MoLow added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 3, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 3, 2023

nodejs-github-bot merged commit d312bd9 into nodejs:main Jul 3, 2023

RafaelGSS mentioned this pull request Jul 3, 2023

v20.4.0 proposal #48643

Merged

ruyadorno mentioned this pull request Sep 11, 2023

v18.18.0 proposal #49220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: todo, only, skip are expected to return a Promise #48555

test_runner: todo, only, skip are expected to return a Promise #48555

shockerqt commented Jun 26, 2023

nodejs-github-bot commented Jun 26, 2023

MoLow commented Jun 26, 2023 •

edited

Loading

shockerqt commented Jun 26, 2023

atlowChemi commented Jun 26, 2023 •

edited

Loading

shockerqt commented Jun 26, 2023

atlowChemi commented Jun 27, 2023

shockerqt commented Jun 27, 2023

atlowChemi left a comment

nodejs-github-bot commented Jun 27, 2023

nodejs-github-bot commented Jul 1, 2023

nodejs-github-bot commented Jul 2, 2023

nodejs-github-bot commented Jul 3, 2023

Fixes: #48557
PR-URL: #48555
Reviewed-By: Moshe Atlow [email protected]

PR-URL: #48555
Reviewed-By: Moshe Atlow [email protected]

PR-URL: #48555
Reviewed-By: Moshe Atlow [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Jul 3, 2023

test_runner: todo, only, skip are expected to return a Promise #48555

test_runner: todo, only, skip are expected to return a Promise #48555

Conversation

shockerqt commented Jun 26, 2023

nodejs-github-bot commented Jun 26, 2023

MoLow commented Jun 26, 2023 • edited Loading

shockerqt commented Jun 26, 2023

atlowChemi commented Jun 26, 2023 • edited Loading

shockerqt commented Jun 26, 2023

atlowChemi commented Jun 27, 2023

shockerqt commented Jun 27, 2023

atlowChemi left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 27, 2023

nodejs-github-bot commented Jul 1, 2023

nodejs-github-bot commented Jul 2, 2023

nodejs-github-bot commented Jul 3, 2023

Fixes: #48557 PR-URL: #48555 Reviewed-By: Moshe Atlow [email protected]

PR-URL: #48555 Reviewed-By: Moshe Atlow [email protected]

PR-URL: #48555 Reviewed-By: Moshe Atlow [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Jul 3, 2023

MoLow commented Jun 26, 2023 •

edited

Loading

atlowChemi commented Jun 26, 2023 •

edited

Loading

Fixes: #48557
PR-URL: #48555
Reviewed-By: Moshe Atlow [email protected]

PR-URL: #48555
Reviewed-By: Moshe Atlow [email protected]

PR-URL: #48555
Reviewed-By: Moshe Atlow [email protected]