test_runner: avoid running twice tests in describe by MoLow · Pull Request #46888 · nodejs/node

MoLow · 2023-02-28T20:30:39Z

not the fix itself can be understood in the first commit, the second one just unifies the code,
wanted to make review easier so split into two commits

nodejs-github-bot · 2023-02-28T20:30:43Z

Review requested:

@nodejs/test_runner

aduh95

I'm not too sure if returning undefined is the correct move (rather than a PromiseResolve()), is there some use case when it would be useful for the users to make the difference?

Other than that, I think I would word the commit as avoid running tests twice, but I'm not an English native speaker, so if the current order is correct, I'm fine with it :)

MoLow · 2023-02-28T21:55:31Z

I'm not too sure if returning undefined is the correct move (rather than a PromiseResolve()), is there some use case when it would be useful for the users to make the difference?

returning a PromiseResolve() might be confusing, if the promise some times fulfilled immediately, and sometimes when the test completes

ljharb · 2023-02-28T22:12:50Z

That sounds less confusing an API to me than one that only sometimes returns a Promise - the latter is literally what's referred to as "zalgo".

MoLow · 2023-03-02T18:52:30Z

Since I don't feel strongly about returning undefined vs returning a promise that resolve immediately, I have changed the code to return PromiseResolve,
CC @aduh95 @ljharb

MoLow · 2023-03-02T18:56:09Z

@nodejs/test_runner since this is a bug in main I'd like to land it, additional reviews/approvals will be appreciated

ljharb · 2023-03-02T19:02:20Z

-
-function runInParentContext(Factory) {
+function runInParentContext(Factory, addShorthands = true) {
 function run(name, options, fn, overrides) {


Suggested change
functionrun(name,options,fn,overrides){
asyncfunctionrun(name,options,fn,overrides){
doing this instead would ensure it always returns a promise

@ljharb change has caused invalid arguments to reject instead of throw so I will revert this

nodejs-github-bot · 2023-03-02T19:28:11Z

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

nodejs-github-bot · 2023-03-02T20:17:23Z

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

nodejs-github-bot · 2023-03-02T22:06:27Z

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

nodejs-github-bot · 2023-03-03T04:43:46Z

Commit Queue failed

- Loading data for nodejs/node/pull/46888
✔ Done loading data for nodejs/node/pull/46888
----------------------------------- PR info ------------------------------------
Title test_runner: avoid running twice tests in describe (#46888)
Author Moshe Atlow (@MoLow)
Branch MoLow:fix-test-in-describe -> nodejs:main
Labels author ready, needs-ci, dont-land-on-v14.x, test_runner
Commits 1
- test_runner: avoid running twice tests in describe
Committers 1
- Moshe Atlow PR-URL: https://github.com/nodejs/node/pull/46888
Reviewed-By: Benjamin Gruenbaum Reviewed-By: Antoine du Hamel ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46888
Reviewed-By: Benjamin Gruenbaum Reviewed-By: Antoine du Hamel --------------------------------------------------------------------------------
ℹ This PR was created on Tue, 28 Feb 2023 20:30:39 GMT
✔ Approvals: 2
✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/46888#pullrequestreview-1320288015
✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/46888#pullrequestreview-1322529537
✖ Last GitHub CI failed
ℹ Last Full PR CI on 2023-03-02T22:06:27Z: https://ci.nodejs.org/job/node-test-pull-request/50173/
- Querying data for job/node-test-pull-request/50173/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

PR-URL: #46888 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

MoLow · 2023-03-03T05:27:31Z

Landed in a37b72d

PR-URL: #46888 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Feb 28, 2023

MoLow force-pushed the fix-test-in-describe branch from d9021c1 to e3c44e3Compare February 28, 2023 20:32

MoLow changed the title ~~test_runner: avoid running tests nedted under describe~~test_runner: avoid running twice tests in describeFeb 28, 2023

ljharb reviewed Feb 28, 2023
View reviewed changes

Comment threadlib/internal/test_runner/harness.js Outdated

MoLow mentioned this pull request Feb 28, 2023
test_runner: align behavior of it and test #46889
Closed
1 task

aduh95 reviewed Feb 28, 2023
View reviewed changes

benjamingr approved these changes Mar 1, 2023
View reviewed changes

jasnell reviewed Mar 2, 2023
View reviewed changes

Comment threaddoc/api/test.md Outdated

test_runner: avoid running twice tests in describe
95d8d57

MoLow force-pushed the fix-test-in-describe branch from 2c235f2 to 95d8d57Compare March 2, 2023 18:44

MoLow requested a review from aduh95 March 2, 2023 18:52

ljharb approved these changes Mar 2, 2023
View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2023

aduh95 approved these changes Mar 2, 2023
View reviewed changes

MoLow force-pushed the fix-test-in-describe branch from fec80e6 to 95d8d57Compare March 2, 2023 20:02

aduh95 approved these changes Mar 2, 2023
View reviewed changes

MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 2, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2023

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 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 Mar 3, 2023

MoLow added a commit that referenced this pull request Mar 3, 2023
test_runner: avoid running twice tests in describe
a37b72d
PR-URL: #46888 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

MoLow closed this Mar 3, 2023

MoLow deleted the fix-test-in-describe branch March 3, 2023 05:27

MoLow mentioned this pull request Mar 3, 2023
test within describe runs twice #46887
Closed

targos pushed a commit that referenced this pull request Mar 13, 2023
test_runner: avoid running twice tests in describe
95bf734
PR-URL: #46888 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

targos pushed a commit that referenced this pull request Mar 14, 2023
test_runner: avoid running twice tests in describe
debc0ad
PR-URL: #46888 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

targos mentioned this pull request Mar 14, 2023
v19.8.0 release proposal #47087
Merged

danielleadams pushed a commit that referenced this pull request Apr 11, 2023
test_runner: avoid running twice tests in describe
413493c
PR-URL: #46888 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

danielleadams mentioned this pull request Apr 11, 2023
v18.16.0 release proposal #47502
Merged

	functionrun(name,options,fn,overrides){
	asyncfunctionrun(name,options,fn,overrides){

Uh oh!

Uh oh!

Conversation

MoLow commented Feb 28, 2023

Uh oh!

nodejs-github-bot commented Feb 28, 2023

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

MoLow commented Feb 28, 2023

Uh oh!

ljharb commented Feb 28, 2023

Uh oh!

Uh oh!

MoLow commented Mar 2, 2023

Uh oh!

MoLow commented Mar 2, 2023

Uh oh!

ljharbMar 2, 2023

Choose a reason for hiding this comment

Uh oh!

MoLowMar 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 2, 2023

Uh oh!

nodejs-github-bot commented Mar 2, 2023

Uh oh!

nodejs-github-bot commented Mar 2, 2023

Uh oh!

nodejs-github-bot commented Mar 3, 2023

Uh oh!

MoLow commented Mar 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants