fix: use native strictEqual for the internal asserts

[ruddenchaux]

1. the first commit fixes the internal assert
2. the second is a test refactor regrouping the tests by method name
3. the third renames the test titles

Closes #13

[ruddenchaux]

@mcollina can you please review it?

[jsumners]

I don't understand the original issue. Why would this function use a different assertion library than the one provided?

[ruddenchaux]

Currently, if you want to use a custom assertion callback, you have to worry about the internals of the check function and this makes it hard to define custom assertion logic.

Let me show an example.

// index.js
const crypto = require('node:crypto')

function businessLogic(loggerInstance) {
  loggerInstance.info({
    uuid: crypto.randomUUID(),
    foo: 'bar',
    baz: 'buz',
  })
}

module.exports = businessLogic

// test.js
const {test} = require('node:test')
const assert = require('node:assert')
const pino = require('pino')
const pinoTest = require('pino-test')
const businessLogic = require('./index')

test('should log a random generated uuid', async () => {
  const stream = pinoTest.sink();
  const loggerInstance = pino(stream);

  businessLogic(loggerInstance);

  const expected = {
    foo: 'bar',
    baz: 'buz',
  }

  await pinoTest.once(stream, expected, (actual, expected) => {
    assert.ok(/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/.test(actual.uuid))
    assert.strictEqual(actual.foo, expected.foo)
    assert.strictEqual(actual.baz, expected.baz)
  })
})

The test will fail because the custom callback will be called 4 times by the internal check function and the first three times the actual will not be an object that contains the expected keys.

So if you want to do this kind of custom assert, you have to complicate the callback in an inappropriate way:

await pinoTest.once(stream, expected, (actual, expected) => {
  if (actual.uuid) {
    assert.ok(/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/.test(actual.uuid))
    assert.strictEqual(actual.foo, expected.foo)
    assert.strictEqual(actual.baz, expected.baz)
  }
})

[jsumners]

I would say that is due to your "custom callback" not actually being an "assert" function as pinoTest.once is expecting. The docs indicate that assert() is meant to be a function with the same signature as nodeAssert.deepStrictEqual:

pino-test/pino-test.js

Lines 47 to 64 in c0bb52c

	/**
	* Assert that a single log is expected.
	*
	* @param {import('node:stream').Transform} stream The stream to be tested.
	* @param {object} expected The expected value to be tested.
	* @param {function} [assert=nodeAssert.deepStrictEqual] The assert function to be used.
	*
	* @returns A promise that resolves when the expected value is equal to the stream value.
	* @throws If the expected value is not equal to the stream value.
	*
	* @example
	* const stream = pino.test.sink()
	* const logger = pino(stream)
	* logger.info('hello world')
	* const expected = { msg: 'hello world', level: 30 }
	* await pino.test.once(stream, expected)
	*/
	async function once (stream, expected, assert = nodeAssert.deepStrictEqual) {

[ruddenchaux]

Yes, both the documentation and the type should be changed. In my opinion, it is a use case that should be allowed by the library, so I would rethink the once, consecutive and check methods. What do you think?

[jsumners]

I think that is a major change and that the library probably shouldn't have been released at v1.0.0.

[ruddenchaux]

@jsumners My latest feat: allow to pass a callback to once and consecutive commit allows to pass a callback to once and consecutive without any breaking change. Can you review it?

[mcollina]

lgtm

[mcollina]

@jsumners PTAL

[mcollina]

(wrong button, sorry)