assert: add new callTracker.callsWith function

[ErickWendel]

This PR adds a new callTracker.callsWith function and its docs

[ErickWendel]

is it ready to go?

[RafaelGSS]

cc: @nodejs/assert

[nodejs-github-bot]

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

[ljharb]

Suggested change

	### `tracker.callsWith([fn],withArgs[, exact])`
	### `tracker.callsWith([fn], withArgs[, exact])`

it is very strange to have an optional argument before a required one. can that be reevaluated?

[ljharb]

Suggested change

	The `withArgs` will compare whether the function arguments are deep-strict equal
	The `withArgs` will compare whether the function arguments are deep-strict equal.

[ljharb]

Suggested change

	```mjs
	```js

i don't think mjs is a syntax highlighting mode, this is just JS

[ljharb]

is this checking if it's truthy, or if it's > 1? a user doesn't have to pass in a number, does it?

[ljharb]

Suggested change

	details: ArrayPrototypePush([], {
	message,
	operator: 'callsWith',
	stack: genericNodeError()
	})
	details: [{
	message,
	operator: 'callsWith',
	stack: genericNodeError()
	}]

[tniessen]

I don't know if this is a good idea.

• This is a very specific test that is simple to implement in userland:

  tracker.calls((...args) => assert.deepStrictEqual(args, [1, 2, 3]));

• As this is written, the user-defined function still cannot rely on the arguments, and error reporting is not ideal:

  function testBuffer(buffer) {
    buffer.slice(0, 10);
  }
  
  const ct = new assert.CallTracker();
  const fn = ct.callsWith(testBuffer, [Buffer.from([1, 2, 3])]);
  // This throws from within testBuffer:
  fn(null);
  // This is never reached, so callsWith() never produces a visible error:
  ct.verify();

• This is not particularly extendible. What if I want to additionally verify the value of this? What if I want to ignore a specific argument? What if I only want to check the typeof of an argument? What if I want reference equality (instead of deep equality) for a specific argument?

[ErickWendel]

I don't know if this is a good idea.

* This is a very specific test that is simple to implement in userland:
  ```js
  tracker.calls((...args) => assert.deepStrictEqual(args, [1, 2, 3]));
  ```

* As this is written, the user-defined function still cannot rely on the arguments, and error reporting is not ideal:
  ```js
  function testBuffer(buffer) {
    buffer.slice(0, 10);
  }
  
  const ct = new assert.CallTracker();
  const fn = ct.callsWith(testBuffer, [Buffer.from([1, 2, 3])]);
  // This throws from within testBuffer:
  fn(null);
  // This is never reached, so callsWith() never produces a visible error:
  ct.verify();
  ```

* This is not particularly extendible. What if I want to additionally verify the value of `this`? What if I want to ignore a specific argument? What if I only want to check the `typeof` of an argument? What if I want reference equality (instead of deep equality) for a specific argument?

Just a thing, I didn't like either the design of this API so I opened this Issue to make it extendable. I got a green signal to go forward on this implementation so I believe I'd mark this PR as draft until I finish the new design in the next few weeks.

Well, I agree this can be done on the userland however I was wondering to implement something like Sinon Spies does to avoid verbosity and replicated code.

This example you pointed out I believe is something expected. Even using only .calls when an error occurs the .verify results are omitted.

{
  function testBuffer(buffer) {
    buffer.slice(0, 10);
  }
  
  const ct = new assert.CallTracker();
  const fn = ct.calls(testBuffer);
  // This throws from within testBuffer:
  fn(null);
  // This is never reached, so callsWith() never produces a visible error:
  ct.verify();
}

About those questions:

What if I want to additionally verify the value of this? What if I want to ignore a specific argument?

I'm not sure if this is the responsibility of .callsWith. Not sure if Sinon does it as well. I think it's a good idea but I'd put it as an additional option to check / ignore argumeents

What if I only want to check the typeof of an argument?

I would add an option flag to check it but it wasn't planned for the .callsWith

What if I want reference equality (instead of deep equality) for a specific argument?

This is the current behavior. see

node/lib/internal/assert/calltracker.js

Line 81 in 71fc444

const containsExpectArgs = isDeepStrictEqual(context.currentFnArgs, context.expectedArgs);

[ErickWendel]

I'm rolling back this PR to Draft as the #43341 will change the function's signature. As soon as it's landed I'll came back here

[tniessen]

Well, I agree this can be done on the userland however I was wondering to implement something like Sinon Spies does to avoid verbosity and replicated code.

Sinon Spies are a good example of a userland implementation. That being said, Sinon's withArgs() is much more powerful than what is being proposed here, so this proposal is not going to replace Sinon Spies.

This example you pointed out I believe is something expected. Even using only .calls when an error occurs the .verify results are omitted.

Yes, but .calls() makes no guarantees about the arguments, so the callee cannot rely on the arguments or their types.

My understanding is that the entire purpose of callsWith() is to verify the arguments, but if the callee still cannot rely on the arguments despite explicitly using callsWith() to verify the arguments and their types, then this is not particularly helpful.

What if I want to additionally verify the value of this? What if I want to ignore a specific argument?

I'm not sure if this is the responsibility of .callsWith. Not sure if Sinon does it as well. I think it's a good idea but I'd put it as an additional option to check / ignore argumeents

Sinon has spy.alwaysCalledOn(), for example.

What if I only want to check the typeof of an argument?

I would add an option flag to check it but it wasn't planned for the .callsWith

If every additional feature will require additional options, the complexity of this API will explode rapidly.

What if I want reference equality (instead of deep equality) for a specific argument?

This is the current behavior. see

node/lib/internal/assert/calltracker.js

Line 81 in 71fc444

const containsExpectArgs = isDeepStrictEqual(context.currentFnArgs, context.expectedArgs);

I don't understand; I think isDeepStrictEqual() checks for deep equality, not for reference equality.

[cjihrig]

@ErickWendel it's totally up to you, but you may want to take a look at cjihrig@e484301. It's proof of concept work for adding mocking to Node's test runner. It probably overlaps with this a bit.

[ErickWendel]

@ErickWendel it's totally up to you, but you may want to take a look at cjihrig@e484301. It's proof of concept work for adding mocking to Node's test runner. It probably overlaps with this a bit.

Uh I liked it! I'm in vacation right now. As soon as I'm back I'll take a look at it

[MoLow]

@ErickWendel I think a much more powerful API can be an API exposing an array of calls and their params, this way you can decide what assersions to perform on the arguments,
if for example some of the arguments are not important or the calling order is not important etc.

I thought of something like this

const tracked = callTracker.track(() => {});
assert.strictEqual(tracked.calls.length, 0);
tracked('bar');
assert.strictEqual(tracked.calls.length, 1);
assert.strictEqual(tracked.calls[0], { arguments: ['bar'] });

WDYT?

[ErickWendel]

@ErickWendel I think a much more powerful API can be an API exposing an array of calls and their params, this way you can decide what assersions to perform on the arguments, if for example some of the arguments are not important or the calling order is not important etc.

I thought of something like this

const tracked = callTracker.track(() => {});
assert.strictEqual(tracked.calls.length, 0);
tracked('bar');
assert.strictEqual(tracked.calls.length, 1);
assert.strictEqual(tracked.calls[0], { arguments: ['bar'] });

WDYT?

wooww I loved the idea. I think Sinon does the same thing right?

[MoLow]

@ErickWendel I think a much more powerful API can be an API exposing an array of calls and their params, this way you can decide what assersions to perform on the arguments, if for example some of the arguments are not important or the calling order is not important etc.
I thought of something like this

const tracked = callTracker.track(() => {});
assert.strictEqual(tracked.calls.length, 0);
tracked('bar');
assert.strictEqual(tracked.calls.length, 1);
assert.strictEqual(tracked.calls[0], { arguments: ['bar'] });

WDYT?

wooww I loved the idea. I think Sinon does the same thing right?

Cool I implemented some POC MoLow@ecd3ff7

[ErickWendel]

closing this PR as it has another one in progress with the PoC MoLow@ecd3ff7

[MoLow]

@ErickWendel can you link to the other PR?

[ErickWendel]

@ErickWendel can you link to the other PR?

just linked your PoC and realized it's not a PR, is it?

[MoLow]

just linked your PoC and realized it's not a PR, is it?

Ah, I did not want to open a PR before you told me you are ok with it. I will rebase my branch and open a PR

[ErickWendel]

just linked your PoC and realized it's not a PR, is it?

Ah, I did not want to open a PR before you told me you are ok with it. I will rebase my branch and open a PR

oh perfect! Sorry for the mess 😂