Node: `error` parameter of `assert.doesNotThrow` should also support types `Object` and `Error`

[RaisinTen]

Is your feature request related to a problem? Please describe.

In this commit, I was trying to assert that the functions being tested don't throw TypeErrors.

Ref: https://github.com/nodejs/node/pull/36190/commits/b640874d5c73aa92791677d1139385e6394de055

This part throws the following error:

// eslint-disable-next-line no-restricted-syntax
assert.doesNotThrow(() => {
  fs.readSync(fd,
              Buffer.allocUnsafe(expected.length),
              0,
              expected.length,
              2n ** 63n - 1n);
}, {
  code: 'ERR_INVALID_ARG_TYPE',
  name: 'TypeError'
});

node:assert:786
    throw new ERR_INVALID_ARG_TYPE(
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "expected" argument must be of type function or an instance of RegExp. Received an instance of Object
    at new NodeError (node:internal/errors:278:15)
    at hasMatchingError (node:assert:786:11)
    at expectsNoError (node:assert:809:17)
    at Function.doesNotThrow (node:assert:834:3)
    at Object.<anonymous> (/home/runner/work/node/node/test/parallel/test-fs-read-type.js:251:8)
    at Module._compile (node:internal/modules/cjs/loader:1102:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1131:10)
    at Module.load (node:internal/modules/cjs/loader:967:32)
    at Function.Module._load (node:internal/modules/cjs/loader:807:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12) {
  code: 'ERR_INVALID_ARG_TYPE'
}
Command: out/Release/node /home/runner/work/node/node/test/parallel/test-fs-read-type.js

but the following code doesn't throw any error:

// eslint-disable-next-line no-restricted-syntax
assert.doesNotThrow(() => {
  fs.read(fd,
          Buffer.allocUnsafe(expected.length),
          0,
          expected.length,
          2n ** 63n - 1n,
          common.mustCall());
}, {
  code: 'ERR_INVALID_ARG_TYPE',
  name: 'TypeError'
});

This is a confusing because assert.throws supports Objects as an error value but assert.doesNotThrow doesn't. Also, I don't know why node isn't throwing an error for the second case given that the effective area of the code the commit is dealing with is just the same.

Describe the solution you'd like

Currently, assert.doesNotThrow only supports the following types for error:

• RegExp
• Function

However, assert.throws supports even more types for error:

• RegExp
• Function
• Object
• Error

It would be better to have the api to be a bit more consistent by adding support for all the types supported by assert.throws in assert.doesNotThrow.

[Lxxyx]

but the following code doesn't throw any error:

What you provide in the example is still assert.doesNotThrow, typo error?

[RaisinTen]

Not a typo, both are assert.doesNotThrow. The second code snippet was placed before the first one in the file and node seemed to notice the bug only in the second snippet.

[Lxxyx]

@RaisinTen
The second case is asynchronous, and the asynchronous call defaults to not reporting an error in the doesNotThrow code.

If you want to test an asynchronous operation, you can use assert.doesNotReject

reference code:
https://github.com/nodejs/node/blob/709b3398782e33924f8dd6f6162bc704a439e3ee/lib/assert.js#L695-L706
https://github.com/nodejs/node/blob/709b3398782e33924f8dd6f6162bc704a439e3ee/lib/assert.js#L814-L816

Also, I agree with @BridgeAR that the API is very strange and not easy to understand.

Demo

const assert = require('assert')

try {
  assert.doesNotThrow(() => {
    setTimeout(() => {
      throw new TypeError('Wrong value')
    }, 1000)
  }, {})
} catch (error) {
  console.log('async error', error)
}

try {
  assert.doesNotThrow(() => {
    throw new TypeError('Wrong value')
  }, {});
} catch (error) {
  console.log('sync error: \n', error)
}

Output

sync error: 
 TypeError [ERR_INVALID_ARG_TYPE]: The "expected" argument must be one of type Function or RegExp. Received type object
    at hasMatchingError (assert.js:737:11)
    at expectsNoError (assert.js:760:17)
    at Function.doesNotThrow (assert.js:785:3)
    at Object.<anonymous> 
    at Module._compile (internal/modules/cjs/loader.js:868:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:879:10)
    at Module.load (internal/modules/cjs/loader.js:731:32)
    at Function.Module._load (internal/modules/cjs/loader.js:644:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:931:10)
    at internal/main/run_main_module.js:17:11
C:\Users\tempCodeRunnerFile.javascript:6
      throw new TypeError('Wrong value')
      ^

TypeError: Wrong value
    at Timeout._onTimeout 
    at listOnTimeout (internal/timers.js:531:17)
    at processTimers (internal/timers.js:475:7)

[BridgeAR]

It would be better to have the api to be a bit more consistent by adding support for all the types supported by assert.throws in assert.doesNotThrow.

I disagree: the doesNotThrow API does not properly use the second argument at all. The API seems confusing, since it's just overhead without providing real benefit to the user. It is a very different API than throws.

What should the following for example log? I would not even know what the second argument should stand for:

assert.doesNotThrow(() => {
  throw new Error('foobar')
}, {
  property: true
})

It will fail due to throwing an error. But what should the object check? It can't say e.g., Expected no error, especially none with 'property === true'.
The API is fundamentally broken and should therefore not be changed anymore.

Also, I don't know why node isn't throwing an error for the second case given that the effective area of the code the commit is dealing with is just the same.

Both examples trigger an ERR_INVALID_ARG_TYPE error for me?

[RaisinTen]

Thank you so much for the help both of you! It's clear to me now. I agree that we shouldn't change the assert API anymore. :)