From c074724fad48fd7a641337376d7897a9854939b3 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Thu, 24 Nov 2022 12:40:41 -0300 Subject: [PATCH 01/19] test: fix mock.method to support class instances It fixes a problem when trying to spy a method from a class instance or static functions on a class instance --- lib/internal/test_runner/mock.js | 38 +++++++++++++---- test/parallel/test-runner-mocking.js | 62 ++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 9 deletions(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index 41bc3b7520431e..235f34d6e07ff9 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -6,6 +6,7 @@ const { FunctionPrototypeCall, ObjectDefineProperty, ObjectGetOwnPropertyDescriptor, + ObjectGetPrototypeOf, Proxy, ReflectApply, ReflectConstruct, @@ -131,8 +132,13 @@ class MockTracker { implementation = kDefaultFunction, options = kEmptyObject, ) { - validateObject(object, 'object'); validateStringOrSymbol(methodName, 'methodName'); + // In case object is a static class + if (typeof object === 'function') { + validateFunction(object, methodName); + } else { + validateObject(object, 'object'); + } if (implementation !== null && typeof implementation === 'object') { options = implementation; @@ -157,16 +163,30 @@ class MockTracker { 'options.setter', setter, "cannot be used with 'options.getter'" ); } + const getOriginalObject = (descriptor) => { + let original; + + if (getter) { + original = descriptor?.get; + } else if (setter) { + original = descriptor?.set; + } else { + original = descriptor?.value; + } + + return original; + }; + let descriptor = ObjectGetOwnPropertyDescriptor(object, methodName); + let original = getOriginalObject(descriptor); - const descriptor = ObjectGetOwnPropertyDescriptor(object, methodName); - let original; + // classes instances + if (typeof original !== 'function') { + descriptor = ObjectGetOwnPropertyDescriptor( + ObjectGetPrototypeOf(object), + methodName + ); - if (getter) { - original = descriptor?.get; - } else if (setter) { - original = descriptor?.set; - } else { - original = descriptor?.value; + original = getOriginalObject(descriptor); } if (typeof original !== 'function') { diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index 82f25aeb3eab71..68937abb1932e6 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -318,6 +318,68 @@ test('spy functions can be bound', (t) => { assert.strictEqual(sum.mock.restore(), undefined); assert.strictEqual(sum.bind(0)(2, 11), 13); }); +test('spies on async class functions', async (t) => { + class Runner { + async someTask(msg) { + return Promise.resolve(msg); + } + + async method(msg) { + await this.someTask(msg); + return msg; + } + } + const msg = 'ok'; + const obj = new Runner(); + assert.strictEqual(await obj.method(msg), msg); + + t.mock.method(obj, obj.someTask.name); + assert.strictEqual(obj.someTask.mock.calls.length, 0); + + assert.strictEqual(await obj.method(msg), msg); + + const call = obj.someTask.mock.calls[0]; + + assert.deepStrictEqual(call.arguments, [msg]); + assert.strictEqual(await call.result, msg); + assert.strictEqual(call.target, undefined); + assert.strictEqual(call.this, obj); + + assert.strictEqual(obj.someTask.mock.restore(), undefined); + assert.strictEqual(await obj.method(msg), msg); + assert.strictEqual(obj.someTask.mock, undefined); +}); + +test('spies on async class static functions', async (t) => { + class Runner { + static async someTask(msg) { + return Promise.resolve(msg); + } + + static async method(msg) { + await this.someTask(msg); + return msg; + } + } + const msg = 'ok'; + assert.strictEqual(await Runner.method(msg), msg); + + t.mock.method(Runner, Runner.someTask.name); + assert.strictEqual(Runner.someTask.mock.calls.length, 0); + + assert.strictEqual(await Runner.method(msg), msg); + + const call = Runner.someTask.mock.calls[0]; + + assert.deepStrictEqual(call.arguments, [msg]); + assert.strictEqual(await call.result, msg); + assert.strictEqual(call.target, undefined); + assert.strictEqual(call.this, Runner); + + assert.strictEqual(Runner.someTask.mock.restore(), undefined); + assert.strictEqual(await Runner.method(msg), msg); + assert.strictEqual(Runner.someTask.mock, undefined); +}); test('mocked functions report thrown errors', (t) => { const testError = new Error('test error'); From 495cebf297c4e8271290b921da01e059a164d422 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Thu, 24 Nov 2022 13:06:15 -0300 Subject: [PATCH 02/19] added suggestions Co-authored-by: Yagiz Nizipli --- lib/internal/test_runner/mock.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index 235f34d6e07ff9..76303dc5881336 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -163,7 +163,7 @@ class MockTracker { 'options.setter', setter, "cannot be used with 'options.getter'" ); } - const getOriginalObject = (descriptor) => { + function getOriginalObject(descriptor) { let original; if (getter) { From e80de49993a0175cae74e9ab1da7e2b3e569ecf3 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Thu, 24 Nov 2022 13:07:30 -0300 Subject: [PATCH 03/19] test: remove unecessarly if Signed-off-by: Erick Wendel --- lib/internal/test_runner/mock.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index 76303dc5881336..897cd863cdc3fc 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -134,9 +134,7 @@ class MockTracker { ) { validateStringOrSymbol(methodName, 'methodName'); // In case object is a static class - if (typeof object === 'function') { - validateFunction(object, methodName); - } else { + if (typeof object !== 'function') { validateObject(object, 'object'); } @@ -175,7 +173,7 @@ class MockTracker { } return original; - }; + } let descriptor = ObjectGetOwnPropertyDescriptor(object, methodName); let original = getOriginalObject(descriptor); From 628142ecabf996bc925ba85ef54b94f1b7de32d4 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Wed, 30 Nov 2022 14:35:19 -0300 Subject: [PATCH 04/19] test: drop comments and rename tests Signed-off-by: Erick Wendel --- lib/internal/test_runner/mock.js | 2 +- test/parallel/test-runner-mocking.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index 897cd863cdc3fc..77d0e3993674ed 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -133,7 +133,7 @@ class MockTracker { options = kEmptyObject, ) { validateStringOrSymbol(methodName, 'methodName'); - // In case object is a static class + if (typeof object !== 'function') { validateObject(object, 'object'); } diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index 68937abb1932e6..5ea01bf1c57d6d 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -318,7 +318,7 @@ test('spy functions can be bound', (t) => { assert.strictEqual(sum.mock.restore(), undefined); assert.strictEqual(sum.bind(0)(2, 11), 13); }); -test('spies on async class functions', async (t) => { +test('mocks prototype methods on an instance', async (t) => { class Runner { async someTask(msg) { return Promise.resolve(msg); @@ -350,7 +350,7 @@ test('spies on async class functions', async (t) => { assert.strictEqual(obj.someTask.mock, undefined); }); -test('spies on async class static functions', async (t) => { +test('spies on async static class methods', async (t) => { class Runner { static async someTask(msg) { return Promise.resolve(msg); From 7fd649aa98889ac1ab44cb079e60147b9abf9b24 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Wed, 30 Nov 2022 16:09:01 -0300 Subject: [PATCH 05/19] test_runner: ensure that mock.method works with class inheritance --- lib/internal/test_runner/mock.js | 26 ++++++++++------ test/parallel/test-runner-mocking.js | 44 ++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index 77d0e3993674ed..017f3ba6b3e9ce 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -174,19 +174,27 @@ class MockTracker { return original; } - let descriptor = ObjectGetOwnPropertyDescriptor(object, methodName); - let original = getOriginalObject(descriptor); - // classes instances - if (typeof original !== 'function') { - descriptor = ObjectGetOwnPropertyDescriptor( - ObjectGetPrototypeOf(object), - methodName - ); + function findMethodOnPrototypeChain(instance, method, isDescriptor = false) { + const original = isDescriptor ? + instance : + ObjectGetOwnPropertyDescriptor(instance, method); + + if (original && !isDescriptor) { + return original; + } - original = getOriginalObject(descriptor); + const proto = ObjectGetPrototypeOf(instance); + const desc = ObjectGetOwnPropertyDescriptor(proto, method); + if (!desc && proto.name) { + return findMethodOnPrototypeChain(proto, method, true); + } + return desc; } + const descriptor = findMethodOnPrototypeChain(object, methodName); + const original = getOriginalObject(descriptor); + if (typeof original !== 'function') { throw new ERR_INVALID_ARG_VALUE( 'methodName', original, 'must be a method' diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index 5ea01bf1c57d6d..ca6d6e82b668ea 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -318,6 +318,7 @@ test('spy functions can be bound', (t) => { assert.strictEqual(sum.mock.restore(), undefined); assert.strictEqual(sum.bind(0)(2, 11), 13); }); + test('mocks prototype methods on an instance', async (t) => { class Runner { async someTask(msg) { @@ -345,6 +346,13 @@ test('mocks prototype methods on an instance', async (t) => { assert.strictEqual(call.target, undefined); assert.strictEqual(call.this, obj); + const obj2 = new Runner(); + // Ensure that a brand new instance is not mocked + assert.strictEqual( + obj2.someTask.mock, + undefined + ); + assert.strictEqual(obj.someTask.mock.restore(), undefined); assert.strictEqual(await obj.method(msg), msg); assert.strictEqual(obj.someTask.mock, undefined); @@ -381,6 +389,42 @@ test('spies on async static class methods', async (t) => { assert.strictEqual(Runner.someTask.mock, undefined); }); +test('given null to a mock.method it throws a invalid argument error', (t) => { + assert.throws(() => t.mock.method(null, {}), /ERR_INVALID_ARG_TYPE/); +}); + +test('spy functions can be used on classes inheritance', (t) => { + class A { + static someTask(msg) { + return msg; + } + static method(msg) { + return this.someTask(msg); + } + } + class B extends A {} + class C extends B {} + + const msg = 'ok'; + assert.strictEqual(C.method(msg), msg); + + t.mock.method(C, C.someTask.name); + assert.strictEqual(C.someTask.mock.calls.length, 0); + + assert.strictEqual(C.method(msg), msg); + + const call = C.someTask.mock.calls[0]; + + assert.deepStrictEqual(call.arguments, [msg]); + assert.strictEqual(call.result, msg); + assert.strictEqual(call.target, undefined); + assert.strictEqual(call.this, C); + + assert.strictEqual(C.someTask.mock.restore(), undefined); + assert.strictEqual(C.method(msg), msg); + assert.strictEqual(C.someTask.mock, undefined); +}); + test('mocked functions report thrown errors', (t) => { const testError = new Error('test error'); const fn = t.mock.fn(() => { From 1ff9a5f53b851d8680290fb8a99492fb81937503 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Wed, 30 Nov 2022 16:48:21 -0300 Subject: [PATCH 06/19] test: ensure that the original object was reseted after execution --- test/parallel/test-runner-mocking.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index ca6d6e82b668ea..71671f4a004687 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -356,6 +356,7 @@ test('mocks prototype methods on an instance', async (t) => { assert.strictEqual(obj.someTask.mock.restore(), undefined); assert.strictEqual(await obj.method(msg), msg); assert.strictEqual(obj.someTask.mock, undefined); + assert.strictEqual(Runner.prototype.someTask.mock, undefined); }); test('spies on async static class methods', async (t) => { @@ -387,6 +388,8 @@ test('spies on async static class methods', async (t) => { assert.strictEqual(Runner.someTask.mock.restore(), undefined); assert.strictEqual(await Runner.method(msg), msg); assert.strictEqual(Runner.someTask.mock, undefined); + assert.strictEqual(Runner.prototype.someTask, undefined); + }); test('given null to a mock.method it throws a invalid argument error', (t) => { From 7ca9a2b09e05bb17b8cb5639c5f8d9286c8afa8c Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Tue, 6 Dec 2022 16:47:38 -0300 Subject: [PATCH 07/19] test_runner: added aduh95's suggestion to extend null --- test/parallel/test-runner-mocking.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index 71671f4a004687..689aa2511d070c 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -397,7 +397,9 @@ test('given null to a mock.method it throws a invalid argument error', (t) => { }); test('spy functions can be used on classes inheritance', (t) => { - class A { + + // make sure that having a null-prototype doesn't throw our system off + class A extends null { static someTask(msg) { return msg; } From ef3188f1c1bc0c375d99463d454de64f0ff620f8 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Tue, 6 Dec 2022 16:56:33 -0300 Subject: [PATCH 08/19] test_runner: remote proto.name validation on mock --- lib/internal/test_runner/mock.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index 017f3ba6b3e9ce..be653552f96e50 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -186,7 +186,8 @@ class MockTracker { const proto = ObjectGetPrototypeOf(instance); const desc = ObjectGetOwnPropertyDescriptor(proto, method); - if (!desc && proto.name) { + + if (!desc) { return findMethodOnPrototypeChain(proto, method, true); } return desc; From 8104c2ac4a5dccc30d427c279211e1f4036ce08b Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Tue, 6 Dec 2022 16:57:25 -0300 Subject: [PATCH 09/19] test: fix lint --- test/parallel/test-runner-mocking.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index 689aa2511d070c..8158329ebadfc2 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -397,8 +397,7 @@ test('given null to a mock.method it throws a invalid argument error', (t) => { }); test('spy functions can be used on classes inheritance', (t) => { - - // make sure that having a null-prototype doesn't throw our system off + // Makes sure that having a null-prototype doesn't throw our system off class A extends null { static someTask(msg) { return msg; From 73ca8209f09b44b4b50e788d68de77b9dd161b20 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Tue, 6 Dec 2022 17:09:17 -0300 Subject: [PATCH 10/19] test_runner: added cjihrig suggestions to turn fns private --- lib/internal/test_runner/mock.js | 67 ++++++++++++++++---------------- 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index be653552f96e50..be82f51c66e89d 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -161,40 +161,9 @@ class MockTracker { 'options.setter', setter, "cannot be used with 'options.getter'" ); } - function getOriginalObject(descriptor) { - let original; - if (getter) { - original = descriptor?.get; - } else if (setter) { - original = descriptor?.set; - } else { - original = descriptor?.value; - } - - return original; - } - - function findMethodOnPrototypeChain(instance, method, isDescriptor = false) { - const original = isDescriptor ? - instance : - ObjectGetOwnPropertyDescriptor(instance, method); - - if (original && !isDescriptor) { - return original; - } - - const proto = ObjectGetPrototypeOf(instance); - const desc = ObjectGetOwnPropertyDescriptor(proto, method); - - if (!desc) { - return findMethodOnPrototypeChain(proto, method, true); - } - return desc; - } - - const descriptor = findMethodOnPrototypeChain(object, methodName); - const original = getOriginalObject(descriptor); + const descriptor = this.#findMethodOnPrototypeChain(object, methodName); + const original = this.#getOriginalObject({ descriptor, getter, setter }); if (typeof original !== 'function') { throw new ERR_INVALID_ARG_VALUE( @@ -294,6 +263,38 @@ class MockTracker { } } + #getOriginalObject({ descriptor, getter, setter }) { + let original; + + if (getter) { + original = descriptor?.get; + } else if (setter) { + original = descriptor?.set; + } else { + original = descriptor?.value; + } + + return original; + } + + #findMethodOnPrototypeChain(instance, method, isDescriptor = false) { + const original = isDescriptor ? + instance : + ObjectGetOwnPropertyDescriptor(instance, method); + + if (original && !isDescriptor) { + return original; + } + + const proto = ObjectGetPrototypeOf(instance); + const desc = ObjectGetOwnPropertyDescriptor(proto, method); + + if (!desc) { + return this.#findMethodOnPrototypeChain(proto, method, true); + } + return desc; + } + #setupMock(ctx, fnToMatch) { const mock = new Proxy(fnToMatch, { __proto__: null, From fe66804cdcdebf3837d719b82d182dc8d6fee48b Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Tue, 6 Dec 2022 18:05:47 -0300 Subject: [PATCH 11/19] test_runner: validate properties in objects --- lib/internal/test_runner/mock.js | 3 ++- lib/internal/validators.js | 15 +++++++++++++++ test/parallel/test-runner-mocking.js | 12 ++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index be82f51c66e89d..7b6608a36237bb 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -23,6 +23,7 @@ const { kEmptyObject } = require('internal/util'); const { validateBoolean, validateFunction, + validateField, validateInteger, validateObject, } = require('internal/validators'); @@ -133,7 +134,6 @@ class MockTracker { options = kEmptyObject, ) { validateStringOrSymbol(methodName, 'methodName'); - if (typeof object !== 'function') { validateObject(object, 'object'); } @@ -161,6 +161,7 @@ class MockTracker { 'options.setter', setter, "cannot be used with 'options.getter'" ); } + validateField(object, methodName); const descriptor = this.#findMethodOnPrototypeChain(object, methodName); const original = this.#getOriginalObject({ descriptor, getter, setter }); diff --git a/lib/internal/validators.js b/lib/internal/validators.js index a971e6574621c5..0c4b02a5c7027b 100644 --- a/lib/internal/validators.js +++ b/lib/internal/validators.js @@ -478,6 +478,20 @@ function validateLinkHeaderFormat(value, name) { } } +/** + * @param {any} object + * @param {string} name + */ +const validateField = hideStackFrames((object, name) => { + if (object === null || !(name in object)) { + throw new ERR_INVALID_ARG_VALUE( + name, + object, + `the property ${name} does not exists in the object instance` + ); + } +}); + const validateInternalField = hideStackFrames((object, fieldKey, className) => { if (typeof object !== 'object' || object === null || !ObjectPrototypeHasOwnProperty(object, fieldKey)) { throw new ERR_INVALID_ARG_TYPE('this', className, object); @@ -547,4 +561,5 @@ module.exports = { validateAbortSignal, validateLinkHeaderValue, validateInternalField, + validateField, }; diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index 8158329ebadfc2..e0139ef769961a 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -396,6 +396,18 @@ test('given null to a mock.method it throws a invalid argument error', (t) => { assert.throws(() => t.mock.method(null, {}), /ERR_INVALID_ARG_TYPE/); }); +test('it should throw given an inexistent property on a object instance', (t) => { + const expectedMessage = [ + 'The argument \'non-existent\'', + ' the property non-existent', + ' does not exists in the object instance.', + ' Received { abc: 0 }', + ].join(''); + assert.throws(() => t.mock.method({ abc: 0 }, 'non-existent'), { + message: expectedMessage + }); +}); + test('spy functions can be used on classes inheritance', (t) => { // Makes sure that having a null-prototype doesn't throw our system off class A extends null { From e35911f8444e2f90b08285fae316f351191ca205 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Tue, 6 Dec 2022 18:19:47 -0300 Subject: [PATCH 12/19] test_runner: add colin's suggestion to simplify algorithm on mock --- lib/internal/test_runner/mock.js | 67 ++++++++++++++++---------------- 1 file changed, 33 insertions(+), 34 deletions(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index 7b6608a36237bb..dbcf1b10915bf2 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -163,8 +163,8 @@ class MockTracker { } validateField(object, methodName); - const descriptor = this.#findMethodOnPrototypeChain(object, methodName); - const original = this.#getOriginalObject({ descriptor, getter, setter }); + const descriptor = findMethodOnPrototypeChain(object, methodName); + const original = getOriginalObject({ descriptor, getter, setter }); if (typeof original !== 'function') { throw new ERR_INVALID_ARG_VALUE( @@ -264,38 +264,6 @@ class MockTracker { } } - #getOriginalObject({ descriptor, getter, setter }) { - let original; - - if (getter) { - original = descriptor?.get; - } else if (setter) { - original = descriptor?.set; - } else { - original = descriptor?.value; - } - - return original; - } - - #findMethodOnPrototypeChain(instance, method, isDescriptor = false) { - const original = isDescriptor ? - instance : - ObjectGetOwnPropertyDescriptor(instance, method); - - if (original && !isDescriptor) { - return original; - } - - const proto = ObjectGetPrototypeOf(instance); - const desc = ObjectGetOwnPropertyDescriptor(proto, method); - - if (!desc) { - return this.#findMethodOnPrototypeChain(proto, method, true); - } - return desc; - } - #setupMock(ctx, fnToMatch) { const mock = new Proxy(fnToMatch, { __proto__: null, @@ -375,4 +343,35 @@ function validateTimes(value, name) { validateInteger(value, name, 1); } +function getOriginalObject({ descriptor, getter, setter }) { + let original; + + if (getter) { + original = descriptor?.get; + } else if (setter) { + original = descriptor?.set; + } else { + original = descriptor?.value; + } + + return original; +} + +function findMethodOnPrototypeChain(instance, methodName) { + let host = instance; + let descriptor; + + while (host !== null) { + descriptor = ObjectGetOwnPropertyDescriptor(host, methodName); + + if (descriptor) { + break; + } + + host = ObjectGetPrototypeOf(host); + } + + return descriptor; +} + module.exports = { MockTracker }; From 92b68b4c2bfc6bfa9ad7ccf240eb7407a757419d Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Wed, 7 Dec 2022 13:04:50 -0300 Subject: [PATCH 13/19] test_runner: turn fn into inline if Signed-off-by: Erick Wendel --- lib/internal/test_runner/mock.js | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index dbcf1b10915bf2..6acd3eca9ecde1 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -164,7 +164,16 @@ class MockTracker { validateField(object, methodName); const descriptor = findMethodOnPrototypeChain(object, methodName); - const original = getOriginalObject({ descriptor, getter, setter }); + + let original; + + if (getter) { + original = descriptor?.get; + } else if (setter) { + original = descriptor?.set; + } else { + original = descriptor?.value; + } if (typeof original !== 'function') { throw new ERR_INVALID_ARG_VALUE( @@ -343,20 +352,6 @@ function validateTimes(value, name) { validateInteger(value, name, 1); } -function getOriginalObject({ descriptor, getter, setter }) { - let original; - - if (getter) { - original = descriptor?.get; - } else if (setter) { - original = descriptor?.set; - } else { - original = descriptor?.value; - } - - return original; -} - function findMethodOnPrototypeChain(instance, methodName) { let host = instance; let descriptor; From 9659cba4b09c856308f08c3e2a77caef18ee2375 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Wed, 7 Dec 2022 13:09:05 -0300 Subject: [PATCH 14/19] test_runner: when method doesnt exists it shows a message --- lib/internal/test_runner/mock.js | 3 --- lib/internal/validators.js | 15 --------------- test/parallel/test-runner-mocking.js | 5 +---- 3 files changed, 1 insertion(+), 22 deletions(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index 6acd3eca9ecde1..1a82618307b4e3 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -23,7 +23,6 @@ const { kEmptyObject } = require('internal/util'); const { validateBoolean, validateFunction, - validateField, validateInteger, validateObject, } = require('internal/validators'); @@ -161,8 +160,6 @@ class MockTracker { 'options.setter', setter, "cannot be used with 'options.getter'" ); } - validateField(object, methodName); - const descriptor = findMethodOnPrototypeChain(object, methodName); let original; diff --git a/lib/internal/validators.js b/lib/internal/validators.js index 0c4b02a5c7027b..a971e6574621c5 100644 --- a/lib/internal/validators.js +++ b/lib/internal/validators.js @@ -478,20 +478,6 @@ function validateLinkHeaderFormat(value, name) { } } -/** - * @param {any} object - * @param {string} name - */ -const validateField = hideStackFrames((object, name) => { - if (object === null || !(name in object)) { - throw new ERR_INVALID_ARG_VALUE( - name, - object, - `the property ${name} does not exists in the object instance` - ); - } -}); - const validateInternalField = hideStackFrames((object, fieldKey, className) => { if (typeof object !== 'object' || object === null || !ObjectPrototypeHasOwnProperty(object, fieldKey)) { throw new ERR_INVALID_ARG_TYPE('this', className, object); @@ -561,5 +547,4 @@ module.exports = { validateAbortSignal, validateLinkHeaderValue, validateInternalField, - validateField, }; diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index e0139ef769961a..f5ffe85a559945 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -398,10 +398,7 @@ test('given null to a mock.method it throws a invalid argument error', (t) => { test('it should throw given an inexistent property on a object instance', (t) => { const expectedMessage = [ - 'The argument \'non-existent\'', - ' the property non-existent', - ' does not exists in the object instance.', - ' Received { abc: 0 }', + 'The argument \'methodName\' must be a method. Received undefined', ].join(''); assert.throws(() => t.mock.method({ abc: 0 }, 'non-existent'), { message: expectedMessage From a6cebb6a24f0c8d78ea060f2f1b346b3d401a31e Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Mon, 12 Dec 2022 10:16:34 -0300 Subject: [PATCH 15/19] add aduh95 suggestion test/parallel/test-runner-mocking.js Co-authored-by: Antoine du Hamel --- test/parallel/test-runner-mocking.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index f5ffe85a559945..c47fce42520d96 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -393,7 +393,7 @@ test('spies on async static class methods', async (t) => { }); test('given null to a mock.method it throws a invalid argument error', (t) => { - assert.throws(() => t.mock.method(null, {}), /ERR_INVALID_ARG_TYPE/); + assert.throws(() => t.mock.method(null, {}), { code: 'ERR_INVALID_ARG_TYPE' }); }); test('it should throw given an inexistent property on a object instance', (t) => { From c075f0e271723ca193156de6efdc60d321d59b8d Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Mon, 12 Dec 2022 10:40:37 -0300 Subject: [PATCH 16/19] test: assert by code instead of regex string --- test/parallel/test-runner-mocking.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index c47fce42520d96..f55d1ebebbfec6 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -2,7 +2,6 @@ const common = require('../common'); const assert = require('node:assert'); const { mock, test } = require('node:test'); - test('spies on a function', (t) => { const sum = t.mock.fn((arg1, arg2) => { return arg1 + arg2; @@ -397,11 +396,8 @@ test('given null to a mock.method it throws a invalid argument error', (t) => { }); test('it should throw given an inexistent property on a object instance', (t) => { - const expectedMessage = [ - 'The argument \'methodName\' must be a method. Received undefined', - ].join(''); assert.throws(() => t.mock.method({ abc: 0 }, 'non-existent'), { - message: expectedMessage + code: 'ERR_INVALID_ARG_VALUE' }); }); From 2edd34e4b9cedd1a3d259db8312cb086362691b4 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Mon, 12 Dec 2022 11:58:58 -0300 Subject: [PATCH 17/19] test_runner: rename object Signed-off-by: Erick Wendel --- lib/internal/test_runner/mock.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/internal/test_runner/mock.js b/lib/internal/test_runner/mock.js index 1a82618307b4e3..7e36930629e1f0 100644 --- a/lib/internal/test_runner/mock.js +++ b/lib/internal/test_runner/mock.js @@ -127,14 +127,14 @@ class MockTracker { } method( - object, + objectOrFunction, methodName, implementation = kDefaultFunction, options = kEmptyObject, ) { validateStringOrSymbol(methodName, 'methodName'); - if (typeof object !== 'function') { - validateObject(object, 'object'); + if (typeof objectOrFunction !== 'function') { + validateObject(objectOrFunction, 'object'); } if (implementation !== null && typeof implementation === 'object') { @@ -160,7 +160,7 @@ class MockTracker { 'options.setter', setter, "cannot be used with 'options.getter'" ); } - const descriptor = findMethodOnPrototypeChain(object, methodName); + const descriptor = findMethodOnPrototypeChain(objectOrFunction, methodName); let original; @@ -178,7 +178,7 @@ class MockTracker { ); } - const restore = { descriptor, object, methodName }; + const restore = { descriptor, object: objectOrFunction, methodName }; const impl = implementation === kDefaultFunction ? original : implementation; const ctx = new MockFunctionContext(impl, restore, times); @@ -200,7 +200,7 @@ class MockTracker { mockDescriptor.value = mock; } - ObjectDefineProperty(object, methodName, mockDescriptor); + ObjectDefineProperty(objectOrFunction, methodName, mockDescriptor); return mock; } From f543e57b68e2880daa1b0b56acbe22674578d00c Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Tue, 13 Dec 2022 12:14:38 -0300 Subject: [PATCH 18/19] test_runner: add test case to check prototype chain Signed-off-by: Erick Wendel --- test/parallel/test-runner-mocking.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index f55d1ebebbfec6..a55446ccc10c3d 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -434,6 +434,31 @@ test('spy functions can be used on classes inheritance', (t) => { assert.strictEqual(C.someTask.mock, undefined); }); +test('spy functions don\'t affect the prototype chain', (t) => { + + class A { + static someTask(msg) { + return msg; + } + } + class B extends A {} + class C extends B {} + + const msg = 'ok'; + + t.mock.method(C, C.someTask.name); + C.someTask(msg); + + assert.strictEqual(B.someTask.mock, undefined); + assert.strictEqual(A.someTask.mock, undefined); + + assert.strictEqual(C.someTask.mock.restore(), undefined); + C.someTask(msg); + assert.strictEqual(C.someTask.mock, undefined); + assert.strictEqual(B.someTask.mock, undefined); + assert.strictEqual(A.someTask.mock, undefined); +}); + test('mocked functions report thrown errors', (t) => { const testError = new Error('test error'); const fn = t.mock.fn(() => { From 152539d4ac32ff10e1eb3152c8c94fe42a409265 Mon Sep 17 00:00:00 2001 From: Erick Wendel Date: Wed, 14 Dec 2022 11:52:45 -0300 Subject: [PATCH 19/19] test: assert property descriptors --- test/parallel/test-runner-mocking.js | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index a55446ccc10c3d..4c48b5a6d83f4f 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -446,17 +446,27 @@ test('spy functions don\'t affect the prototype chain', (t) => { const msg = 'ok'; + const ABeforeMockIsUnchanged = Object.getOwnPropertyDescriptor(A, A.someTask.name); + const BBeforeMockIsUnchanged = Object.getOwnPropertyDescriptor(B, B.someTask.name); + const CBeforeMockShouldNotHaveDesc = Object.getOwnPropertyDescriptor(C, C.someTask.name); + t.mock.method(C, C.someTask.name); C.someTask(msg); + const BAfterMockIsUnchanged = Object.getOwnPropertyDescriptor(B, B.someTask.name); + + const AAfterMockIsUnchanged = Object.getOwnPropertyDescriptor(A, A.someTask.name); + const CAfterMockHasDescriptor = Object.getOwnPropertyDescriptor(C, C.someTask.name); + + assert.strictEqual(CBeforeMockShouldNotHaveDesc, undefined); + assert.ok(CAfterMockHasDescriptor); - assert.strictEqual(B.someTask.mock, undefined); - assert.strictEqual(A.someTask.mock, undefined); + assert.deepStrictEqual(ABeforeMockIsUnchanged, AAfterMockIsUnchanged); + assert.strictEqual(BBeforeMockIsUnchanged, BAfterMockIsUnchanged); + assert.strictEqual(BBeforeMockIsUnchanged, undefined); assert.strictEqual(C.someTask.mock.restore(), undefined); - C.someTask(msg); - assert.strictEqual(C.someTask.mock, undefined); - assert.strictEqual(B.someTask.mock, undefined); - assert.strictEqual(A.someTask.mock, undefined); + const CAfterRestoreKeepsDescriptor = Object.getOwnPropertyDescriptor(C, C.someTask.name); + assert.ok(CAfterRestoreKeepsDescriptor); }); test('mocked functions report thrown errors', (t) => {