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

test_runner: add getter and setter to MockTracker #45506

Closed
wants to merge 1 commit into from

Conversation

fossamagna
Copy link
Contributor

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)

@nodejs-github-bot 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 Nov 18, 2022
Comment on lines 212 to 213
getter: true,
...options
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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() ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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().

doc/api/test.md Outdated
added: REPLACEME
-->

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

Choose a reason for hiding this comment

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

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

doc/api/test.md Outdated
added: REPLACEME
-->

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

Choose a reason for hiding this comment

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

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

@fossamagna fossamagna force-pushed the add-mock-getter-setter branch 2 times, most recently from f16927b to e240af4 Compare November 18, 2022 16:00
doc/api/test.md Outdated
This function is syntax sugar for [`MockTracker.method`][] with `options.getter`
set to `true`.

### `mock.setter(object, methodName[, implementation][, options])`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will sort them in alphabetical order.


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

Choose a reason for hiding this comment

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

Same comment for setter:

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

Comment on lines 216 to 220
const { getter = true } = options;

validateBoolean(getter, 'options.getter');

if (!getter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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) {

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
lib/internal/test_runner/mock.js Show resolved Hide resolved
lib/internal/test_runner/mock.js Show resolved Hide resolved

validateBoolean(setter, 'options.setter');

if (!setter) {
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
if (!setter) {
if (setter === false) {

Comment on lines 216 to 220
const { getter = true } = options;

validateBoolean(getter, 'options.getter');

if (!getter) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

@fossamagna fossamagna force-pushed the add-mock-getter-setter branch 2 times, most recently from cde2736 to 8e13ae4 Compare November 19, 2022 15:04
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.

LGTM

doc/api/test.md Outdated
@@ -941,6 +941,17 @@ test('mocks a counting function', (t) => {
});
```

they are sorted in alphabetical order.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
they are sorted in alphabetical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. I remove this.


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

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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


return this.method(object, methodName, implementation, {
...options,
getter: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
getter: true
getter,

Comment on lines 214 to 216
}

validateObject(options, 'options');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
}
validateObject(options, 'options');
} else {
validateObject(options, 'options');
}

Comment on lines 243 to 245
}

validateObject(options, 'options');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
validateObject(options, 'options');
} else {
validateObject(options, 'options');
}


return this.method(object, methodName, implementation, {
...options,
setter: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
setter: true
setter,

doc/api/test.md Outdated
@@ -941,6 +941,17 @@ test('mocks a counting function', (t) => {
});
```

they are sorted in alphabetical order.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
they are sorted in alphabetical order.

Comment on lines 220 to 221
validateBoolean(getter, 'options.getter');

Copy link
Contributor

@aduh95 aduh95 Nov 20, 2022

Choose a reason for hiding this comment

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

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');

Comment on lines 249 to 250
validateBoolean(setter, 'options.setter');

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Suggested change
validateBoolean(setter, 'options.setter');

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

return this.method(object, methodName, implementation, {
...options,
getter
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
getter
getter,


return this.method(object, methodName, implementation, {
...options,
setter
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2022
@nodejs-github-bot
Copy link
Collaborator

@ljharb
Copy link
Member

ljharb commented Nov 22, 2022

cc @nodejs/testing

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

@cjihrig 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 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
Copy link
Collaborator

Landed in 147d810...afed1af

nodejs-github-bot pushed a commit that referenced this pull request Nov 27, 2022
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>
ErickWendel pushed a commit to ErickWendel/node that referenced this pull request Nov 30, 2022
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>
targos pushed a commit that referenced this pull request Dec 12, 2022
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
Copy link
Member

targos commented Dec 14, 2022

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
Copy link
Member

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
Copy link
Member

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

@targos
Copy link
Member

targos commented Dec 14, 2022

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
Copy link
Member

targos commented Dec 14, 2022

At least that's the case on the main branch

@richardlau
Copy link
Member

Ah it's possible the release commit merged incorrectly.

richardlau added a commit to richardlau/node-1 that referenced this pull request Dec 14, 2022
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
Copy link
Member

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

Opened #45863 to correct.

richardlau added a commit to richardlau/node-1 that referenced this pull request Dec 15, 2022
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
nodejs-github-bot pushed a commit that referenced this pull request Dec 16, 2022
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>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
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>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
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>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
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>
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 2, 2023
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants