doc: update assert.rejects docs with a validation function example

[MadLittleMods]

Update assert.rejects docs with a validation function example

Spawned from my own struggle to use in https://gitlab.com/gitlab-org/gitter/webapp/merge_requests/1702#note_268452483

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[BridgeAR]

Thanks for the PR. The reason why this API only has few examples is that it's easily out of sync with assert.throws() while almost being identical besides the async nature.

That's also indicated in the description: Besides the async nature to await the completion behaves identically to `assert.throws()`

Maybe it's better to reference that for further examples instead?

[MadLittleMods]

Perhaps it is mainly due to the fact I am not familiar with assert.throws

Is the assert.throws() API really changing that much to go out of sync?

We could add a note like "For more examples, see assert.throws()" but it does feel a bit redundant to the note above about it behaving identically. I'm not really sure about the best clarification if we don't want an extra example added. Just know I failed to follow the cross-link to assert.throws in the current state.

Appreciate the quick response!

[BridgeAR]

It's not about it changing frequently but also about adding a lot of duplication.
I do agree that it would be good to further improve the documentation and it might be the most intuitive thing to duplicate all examples.

[MadLittleMods]

@BridgeAR Friendly poke. I'm in favor of merging but huge bias on my end. This example in particular was the most valuable and I could work from here.

If not, please close

[addaleax]

@MadLittleMods I’m good with merging this, but can you fix the linter failures?

[MadLittleMods]

@addaleax Thanks for the nudge! Everything is passing now ✔️

[Trott]

The example code in this PR would result in an unhandled promise rejection if one of the assertions in the validation function is triggered. This means the code does not exit with an error code.

For example, here's that sample code with one line added (to load the assert module) and one line changed (to cause the assertion to fail):

const assert = require('assert');

(async () => {
  await assert.rejects(
    async () => {
      throw new TypeError('Wrong value');
    },
    (err) => {
      assert.strictEqual(err.name, 'TypeError');
      assert.strictEqual(err.message, 'Oops, this will throw!');
      return true;
    }
  );
})();

Put that code in test.js and then run:

node test2.js ; echo "Exit code: $?"

The result will look like this:

(node:32509) UnhandledPromiseRejectionWarning: AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ 'Wrong value'
- 'Oops, this will throw!'
    at Object.<anonymous> (/Users/trott/io.js/test2.js:10:14)
    at expectedException (assert.js:653:26)
    at expectsError (assert.js:778:3)
    at Function.rejects (assert.js:831:3)
    at async /Users/trott/io.js/test2.js:4:3
(node:32509) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
(node:32509) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
Exit code: 0

The output looks like it's failing, but the exit code is 0. So a test runner most likely will treat this as a passing test. 😲

At a minimum, I think we should provide an example that will exit with an error code when an assertion fails.

[Trott]

Welcome, @MadLittleMods and thanks for the pull request! I have a concern that this code does not cause an error when an assertion in the validation function fails. This is because the resulting promise rejection is unhandled.

The fact that an assertion failure does not result in an error code will be surprising to many end users I think. Because users often copy code from the docs expecting it to be model code, I think it's important that we avoid such hidden gotchas. (This could introduce tests that appear to work but don't actually fail when they are supposed to fail.) Would you mind updating the example to appropriately handle the promise rejection (while still keeping the code as simple as is possible given the constraints)?

[MadLittleMods]

@Trott I was following the example just above it which seems to perform the same if we make it fail. How should we present these better?

[Trott]

@Trott I was following the example just above it which seems to perform the same if we make it fail. How should we present these better?

This seems like a bug in assert.rejects()? Even if I change it to return false, it still prints the error but exits with a success code. @nodejs/assert

[cjihrig]

Since assert.rejects() deals in promises, isn't this working as expected? A rejected promise doesn't crash the process like an uncaught exception would. If you run node with --unhandled-rejections=strict then you see the exit code turn to 1. Similarly, if you had a failed assert.throws() but added a process.on('uncaughtException, ...) handler, you see the exit code become 0 instead of 1.

[Trott]

Since assert.rejects() deals in promises, isn't this working as expected? A rejected promise doesn't crash the process like an uncaught exception would. If you run node with --unhandled-rejections=strict then you see the exit code turn to 1. Similarly, if you had a failed assert.throws() but added a process.on('uncaughtException, ...) handler, you see the exit code become 0 instead of 1.

I would think users are going to expect that the validation function failing will result in the test failing, meaning the process should exit with a non-zero code. Even if the current behavior is correct, the example code currently in the docs and the example code added here does not do that.

It seems to me that even if assert.rejects() is acting as designed, that this is going to be surprising to a lot of users.

And it seems to me that we should avoid having sample code that prints an assertion error but exits with a zero status code. People are going to copy it into their tests, thinking that it will result in a failing status code if the test fails, and it won't.

[cjihrig]

I'm not necessarily disagreeing with you @Trott. I think the behavior is confusing/misleading as well, but does seem to be technically correct. I think we should improve the documentation to point that out why that happens.

People are going to copy it into their tests, thinking that it will result in a failing status code if the test fails, and it won't.

That might depend on the test runner. I just copied the example into a lab test and it failed and exited with a code of 1.

[MadLittleMods]

It fails tests as expected in Mocha and Jest too which have the hooks to fail the process.

Adding a note about running in --unhandled-rejections=strict could be cool to add. Any objections?

[Trott]

@cjihrig @MadLittleMods Can you share what your test code looks like that is working correctly with jest/mocha/lab?

[cjihrig]

Sure. See the details block below. I'm seeing the expected failures when running individual or multiple tests. You have to run via the @hapi/lab test runner though, not via Node directly (node test.js).

'use strict';
const Assert = require('assert');
const Lab = require('@hapi/lab');
const { it } = exports.lab = Lab.script();

it('fails as expected', async () => {
  await Assert.rejects(
    async () => {
      throw new TypeError('Wrong value');
    },
    (err) => {
      Assert.strictEqual(err.name, 'TypeError');
      Assert.strictEqual(err.message, 'Oops, this will throw!');
      return true;
    }
  );
});

it('fails as expected', () => {
  return Assert.rejects(
    async () => {
      throw new TypeError('Wrong value');
    },
    (err) => {
      Assert.strictEqual(err.name, 'TypeError');
      Assert.strictEqual(err.message, 'Oops, this will throw!');
      return true;
    }
  );
});

it('fails as expected', async () => {
  await Assert.rejects(
    async () => {
      throw new TypeError('Wrong value');
    },
    (err) => {
      return false;
    }
  );
});

it('fails as expected', () => {
  return Assert.rejects(
    async () => {
      throw new TypeError('Wrong value');
    },
    (err) => {
      return false;
    }
  );
});

[MadLittleMods]

@Trott

Here is some real world usage in Mocha:

• modules/permissions/test/ensure-access-and-fetch-descriptor-test.js#L121-133 -> npm run mocha -- modules/permissions/test/ensure-access-and-fetch-descriptor-test.js

---

And just tested this in Jest:

✔️ Passing example:

const assert = require('assert');

describe('my thing', () => {
  it('my test', async () => {
    await assert.rejects(
      async () => {
        throw new TypeError('Wrong value');
      },
      err => {
        assert.strictEqual(err.name, 'TypeError');
        assert.strictEqual(err.message, 'Wrong value');
        return true;
      }
    );
  });
});

❌ Failing example:

const assert = require('assert');

describe('my thing', () => {
  it('my test', async () => {
    await assert.rejects(
      async () => {
        throw new TypeError('Wrong value');
      },
      err => {
        assert.strictEqual(err.name, 'TypeError');
        assert.strictEqual(err.message, 'DOES NOT MATCH');
        return true;
      }
    );
  });
});

$ npm run jest -- asdf

  my thing
    × my test (7ms)

  ● my thing › my test

    assert.strictEqual(received, expected)

    Expected value to strictly be equal to:
      "DOES NOT MATCH"
    Received:
      "Wrong value"

    Difference:

    - Expected
    + Received

    - DOES NOT MATCH
    + Wrong value

       9 |       err => {
      10 |         assert.strictEqual(err.name, 'TypeError');
    > 11 |         assert.strictEqual(err.message, 'DOES NOT MATCH');
         |                ^
      12 |         return true;
      13 |       }
      14 |     );

      at Object.strictEqual (public/js/vue/left-menu/components/asdf-test.js:11:16)

[Trott]

Since the problem I'm concerned about is already there in the existing example code for assert.rejects(), I'm going to clear the request for changes and make a note for myself to figure out what (if anything) to do at a later date.

It's good that test runners do the right thing. Still, lots of packages don't use test runners and just run a test.js file directly, expecting it to fail when an AssertionError happens. I'm still uncomfortable with the idea that we have runnable code in our examples that have assertions that, if they were to fail, would print an AssertionError but exit with a non-zero status code.

problem is pre-existing, can be fixed in another PR

[Trott]

Landed in be2f3a3.

Thanks for the contribution and your patience! 🎉

[MadLittleMods]

Great to see this merged! 🚀

Thanks for the review and help all 🤗