test_runner: add getter and setter to MockTracker #45506

fossamagna · 2022-11-18T09:33:15Z

This commit allows tests in test runner to use the getter and setter methods as "syntax sugar" for MockTracker.method with the options.getter or options.setter set to true in the options.

Refs: #45326 (comment)

lib/internal/test_runner/mock.js

cjihrig · 2022-11-18T14:11:51Z

lib/internal/test_runner/mock.js

+      getter: true,
+      ...options


Can you swap these two lines so that getter cannot be set to a different value. Same comment below for setter.

cjihrig · 2022-11-18T14:13:43Z

test/parallel/test-runner-mocking.js

+  function mockMethod() {
+    return this.prop - 1;
+  }
+  const getter = t.mock.getter(obj, 'method', mockMethod);


Can you add tests that try to set getter and setter to false. Tests that throw when trying to set setter: true when calling getter() and vice versa would be nice to.

@cjihrig Thank you for your suggestions.
If getter cannot be set to a different value, I think that validation do not work for options in method().
Do we implement validation for options in getter() and setter() ?

If getter cannot be set to a different value, I think that validation do not work for options in method().

There is validation, but if you set the getter option to false, it would pass validation, but the getter() method would not make sense.

Do we implement validation for options in getter() and setter() ?

I would be OK with validating the getter or setter option to ensure that they are not set to any value besides true and deferring all other validation to method().

cjihrig · 2022-11-18T14:14:09Z

doc/api/test.md

+added: REPLACEME
+-->
+
+This function is syntax sugar for [`MockTracker.method`][] with `options.getter` is `true`.


Suggested change

This function is syntax sugar for [`MockTracker.method`][] with `options.getter` is `true`.

This function is syntax sugar for [`MockTracker.method`][] with `options.getter` set to `true`.

cjihrig · 2022-11-18T14:14:26Z

doc/api/test.md

+added: REPLACEME
+-->
+
+This function is syntax sugar for [`MockTracker.method`][] with `options.setter` is `true`.


Suggested change

This function is syntax sugar for [`MockTracker.method`][] with `options.setter` is `true`.

This function is syntax sugar for [`MockTracker.method`][] with `options.setter` set to `true`.

cjihrig · 2022-11-18T16:56:27Z

doc/api/test.md

+This function is syntax sugar for [`MockTracker.method`][] with `options.getter`
+set to `true`.
+
+### `mock.setter(object, methodName[, implementation][, options])`


Sorry, I just realized that this is not sorted. mock.setter() should come after mock.reset().

I will sort them in alphabetical order.

cjihrig · 2022-11-18T17:02:04Z

lib/internal/test_runner/mock.js

+
+    if (!getter) {
+      throw new ERR_INVALID_ARG_VALUE(
+        'options.getter', getter, "cannot set to 'false'"


Same comment for setter:

Suggested change

'options.getter', getter, "cannot set to 'false'"

'options.getter', getter, "cannot be false"

cjihrig · 2022-11-18T17:04:15Z

lib/internal/test_runner/mock.js

+    const { getter = true } = options;
+
+    validateBoolean(getter, 'options.getter');
+
+    if (!getter) {


We can simplify this and let method() handle the validation (same for setter).

Suggested change

const { getter = true } = options;

validateBoolean(getter, 'options.getter');

if (!getter) {

if (options?.getter === false) {

I didn't understand the reason/logic behind allowing only getter === true while setting it to true as default, but if it's false throw an error? Why do we even check for options?.getter at all?

It is incorrect to specify options.getter for getter(). If we don't check it, getter() will not throw an error even if we specify false for options.getter. I think that it is better to check options.getter, since getter() will not throw an error if options.getter is specified as false.

lib/internal/test_runner/mock.js

anonrig · 2022-11-18T23:00:40Z

lib/internal/test_runner/mock.js

+
+    validateBoolean(setter, 'options.setter');
+
+    if (!setter) {


Suggested change

if (!setter) {

if (setter === false) {

anonrig · 2022-11-18T23:02:10Z

lib/internal/test_runner/mock.js

+    const { getter = true } = options;
+
+    validateBoolean(getter, 'options.getter');
+
+    if (!getter) {


I didn't understand the reason/logic behind allowing only getter === true while setting it to true as default, but if it's false throw an error? Why do we even check for options?.getter at all?

MoLow

LGTM

cjihrig · 2022-11-20T05:18:34Z

doc/api/test.md

@@ -941,6 +941,17 @@ test('mocks a counting function', (t) => {
 });
 ```

+they are sorted in alphabetical order.


Please remove this.

Suggested change

they are sorted in alphabetical order.

Sorry. I remove this.

cjihrig · 2022-11-20T05:21:22Z

lib/internal/test_runner/mock.js

+
+    if (getter === false) {
+      throw new ERR_INVALID_ARG_VALUE(
+        'options.getter', getter, "cannot be 'false'"


Suggested change

'options.getter', getter, "cannot be 'false'"

'options.getter', getter, "cannot be false"

cjihrig · 2022-11-20T05:21:57Z

lib/internal/test_runner/mock.js

+
+    if (setter === false) {
+      throw new ERR_INVALID_ARG_VALUE(
+        'options.setter', setter, "cannot be 'false'"


Suggested change

'options.setter', setter, "cannot be 'false'"

'options.setter', setter, "cannot be false"

aduh95 · 2022-11-20T08:20:42Z

lib/internal/test_runner/mock.js

+
+    return this.method(object, methodName, implementation, {
+      ...options,
+      getter: true


Suggested change

getter: true

getter,

aduh95 · 2022-11-20T08:21:37Z

lib/internal/test_runner/mock.js

+    }
+
+    validateObject(options, 'options');


We can skip the validation if we know it's an object already.

Suggested change

}

validateObject(options, 'options');

} else {

validateObject(options, 'options');

}

aduh95 · 2022-11-20T08:22:25Z

lib/internal/test_runner/mock.js

+    }
+
+    validateObject(options, 'options');


Suggested change

}

validateObject(options, 'options');

} else {

validateObject(options, 'options');

}

aduh95 · 2022-11-20T08:22:37Z

lib/internal/test_runner/mock.js

+
+    return this.method(object, methodName, implementation, {
+      ...options,
+      setter: true


Suggested change

setter: true

setter,

aduh95 · 2022-11-20T08:23:11Z

doc/api/test.md

@@ -941,6 +941,17 @@ test('mocks a counting function', (t) => {
 });
 ```

+they are sorted in alphabetical order.


Suggested change

they are sorted in alphabetical order.

aduh95 · 2022-11-20T08:26:13Z

lib/internal/test_runner/mock.js

+    validateBoolean(getter, 'options.getter');
+


The validation is already done in this.method (if you implement my suggestion in #45506 (comment)), I suggest removing it.

Suggested change

validateBoolean(getter, 'options.getter');

aduh95 · 2022-11-20T08:26:17Z

lib/internal/test_runner/mock.js

+    validateBoolean(setter, 'options.setter');
+


Same here.

Suggested change

validateBoolean(setter, 'options.setter');

aduh95 · 2022-11-20T16:01:58Z

lib/internal/test_runner/mock.js

+
+    return this.method(object, methodName, implementation, {
+      ...options,
+      getter


Suggested change

getter

getter,

aduh95 · 2022-11-20T16:02:08Z

lib/internal/test_runner/mock.js

+
+    return this.method(object, methodName, implementation, {
+      ...options,
+      setter


Suggested change

setter

setter,

This commit allows tests in test runner to use the `getter` and `setter` methods as "syntax sugar" for `MockTracker.method` with the `options.getter` or `options.setter` set to true in the options. Refs: nodejs#45326 (comment)

nodejs-github-bot · 2022-11-21T14:56:01Z

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

ljharb · 2022-11-22T22:53:34Z

cc @nodejs/testing

aduh95

Ideally we would have tests to ensure that passing non-boolean values to getter or setter would trigger an error. Otherwise LGTM.

nodejs-github-bot · 2022-11-23T05:04:05Z

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

nodejs-github-bot · 2022-11-27T23:12:15Z

Landed in 147d810...afed1af

This commit allows tests in test runner to use the `getter` and `setter` methods as "syntax sugar" for `MockTracker.method` with the `options.getter` or `options.setter` set to true in the options. Refs: #45326 (comment) PR-URL: #45506 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

This commit allows tests in test runner to use the `getter` and `setter` methods as "syntax sugar" for `MockTracker.method` with the `options.getter` or `options.setter` set to true in the options. Refs: nodejs#45326 (comment) PR-URL: nodejs#45506 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

This commit allows tests in test runner to use the `getter` and `setter` methods as "syntax sugar" for `MockTracker.method` with the `options.getter` or `options.setter` set to true in the options. Refs: #45326 (comment) PR-URL: #45506 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

targos · 2022-12-14T13:52:21Z

Apparently the documentation for this landed in v16.x (maintenance LTS). I don't see any other references above so maybe the feature itself wasn't backported. /cc @richardlau

richardlau · 2022-12-14T17:43:40Z

Apparently the documentation for this landed in v16.x (maintenance LTS). I don't see any other references above so maybe the feature itself wasn't backported. /cc @richardlau

I'm confused. I don't see any references to mock(.getter/.setter) in https://nodejs.org/docs/v16.19.0/api/test.html.

richardlau · 2022-12-14T17:50:36Z

Mocking itself is one of the commits in the open backport #45602 but that hasn't landed on v16.x-staging yet.

targos · 2022-12-14T19:35:19Z

I'll dig in tomorrow, but while releasing v19.3.0, I had to update the added yaml field, which had an initial value for v16

targos · 2022-12-14T19:35:58Z

At least that's the case on the main branch

richardlau · 2022-12-14T19:53:11Z

Ah it's possible the release commit merged incorrectly.

When cherry-picking the release commit for Node.js 16.19.0 to the `main` branch git updated the metadata for the wrong item in `doc/api/test.md`. The incorrectly updated item has been fixed in a subsequent commit (the merge for the 19.3.0 release commit). This commit updates the intended item that should have been updated when the 16.19.0 release commit was merged. Refs: nodejs#45506 (comment) Refs: nodejs/Release#771

richardlau · 2022-12-14T20:25:14Z

Yes, it was indeed. Another example of nodejs/Release#771 😞. Thanks for catching it.

Opened #45863 to correct.

When cherry-picking the release commit for Node.js 16.19.0 to the `main` branch git updated the metadata for the wrong item in `doc/api/test.md`. Refs: nodejs#45506 (comment) Refs: nodejs/Release#771

When cherry-picking the release commit for Node.js 16.19.0 to the `main` branch git updated the metadata for the wrong item in `doc/api/test.md`. Refs: #45506 (comment) Refs: nodejs/Release#771 PR-URL: #45863 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Tierney Cyren <hello@bnb.im> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

This commit allows tests in test runner to use the `getter` and `setter` methods as "syntax sugar" for `MockTracker.method` with the `options.getter` or `options.setter` set to true in the options. Refs: #45326 (comment) PR-URL: #45506 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

This commit allows tests in test runner to use the `getter` and `setter` methods as "syntax sugar" for `MockTracker.method` with the `options.getter` or `options.setter` set to true in the options. Refs: nodejs/node#45326 (comment) PR-URL: nodejs/node#45506 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> (cherry picked from commit afed1afa55962211b6b56c2068d520b4d8a08888)

nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner labels Nov 18, 2022

MoLow reviewed Nov 18, 2022

View reviewed changes

lib/internal/test_runner/mock.js Outdated Show resolved Hide resolved

MoLow reviewed Nov 18, 2022

View reviewed changes

lib/internal/test_runner/mock.js Outdated Show resolved Hide resolved

fossamagna force-pushed the add-mock-getter-setter branch from 04a27b4 to 6dca9b9 Compare November 18, 2022 14:06

cjihrig reviewed Nov 18, 2022

View reviewed changes

fossamagna force-pushed the add-mock-getter-setter branch 2 times, most recently from f16927b to e240af4 Compare November 18, 2022 16:00

cjihrig reviewed Nov 18, 2022

View reviewed changes

anonrig reviewed Nov 18, 2022

View reviewed changes

fossamagna force-pushed the add-mock-getter-setter branch 2 times, most recently from cde2736 to 8e13ae4 Compare November 19, 2022 15:04

MoLow approved these changes Nov 19, 2022

View reviewed changes

cjihrig reviewed Nov 20, 2022

View reviewed changes

aduh95 reviewed Nov 20, 2022

View reviewed changes

fossamagna force-pushed the add-mock-getter-setter branch 2 times, most recently from bae6aa6 to dd8d5d3 Compare November 20, 2022 14:21

aduh95 reviewed Nov 20, 2022

View reviewed changes

test_runner: add getter and setter to MockTracker

6da996a

This commit allows tests in test runner to use the `getter` and `setter` methods as "syntax sugar" for `MockTracker.method` with the `options.getter` or `options.setter` set to true in the options. Refs: nodejs#45326 (comment)

fossamagna force-pushed the add-mock-getter-setter branch from dd8d5d3 to 6da996a Compare November 20, 2022 23:59

cjihrig approved these changes Nov 21, 2022

View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2022

github-actions bot mentioned this pull request Nov 22, 2022

CI Reliability 2022-11-22 nodejs/reliability#436

Open

15 tasks

aduh95 approved these changes Nov 22, 2022

View reviewed changes

github-actions bot mentioned this pull request Nov 23, 2022

CI Reliability 2022-11-23 nodejs/reliability#437

Open

13 tasks

github-actions bot mentioned this pull request Nov 24, 2022

CI Reliability 2022-11-24 nodejs/reliability#438

Open

16 tasks

cjihrig added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed needs-ci PRs that need a full CI run. labels Nov 27, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2022

nodejs-github-bot closed this Nov 27, 2022

targos mentioned this pull request Dec 12, 2022

v19.3.0 release proposal #45831

Merged

richardlau mentioned this pull request Dec 14, 2022

doc: fix doc metadata for Node.js 16.19.0 #45863

Merged

danielleadams mentioned this pull request Dec 30, 2022

v18.13.0 release proposal #46025

Merged

	This function is syntax sugar for [`MockTracker.method`][] with `options.getter` is `true`.
	This function is syntax sugar for [`MockTracker.method`][] with `options.getter` set to `true`.

	'options.getter', getter, "cannot set to 'false'"
	'options.getter', getter, "cannot be false"

	'options.getter', getter, "cannot be 'false'"
	'options.getter', getter, "cannot be false"

	'options.setter', setter, "cannot be 'false'"
	'options.setter', setter, "cannot be false"

test_runner: add getter and setter to MockTracker #45506

test_runner: add getter and setter to MockTracker #45506

Conversation

fossamagna commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoLow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 Nov 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 21, 2022

ljharb commented Nov 22, 2022

aduh95 left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 23, 2022

nodejs-github-bot commented Nov 27, 2022

targos commented Dec 14, 2022

richardlau commented Dec 14, 2022

richardlau commented Dec 14, 2022

targos commented Dec 14, 2022

targos commented Dec 14, 2022

richardlau commented Dec 14, 2022

richardlau commented Dec 14, 2022

aduh95 Nov 20, 2022 •

edited