assert: add env to show the full error

[BridgeAR]

This adds the NODE_ASSERT_FULL environment variable to give the
user the option to opt into seeing the full error message even though
the diff is at least partially identical.

Fixes: #19106

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

[nodejs-github-bot]

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/420/pipeline

[vsemozhetbyt]

Nit: an loose -> a loose.

[vsemozhetbyt]

It seems these 3 files should also be updated?

https://github.com/nodejs/node/blob/master/doc/api/cli.md#environment-variables

node/doc/node.1

Line 274 in 0518b9e

.Sh ENVIRONMENT

node/src/node.cc

Line 2580 in 0518b9e

static void PrintHelp() {

[BridgeAR]

Comments addressed. PTAL.

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

[vsemozhetbyt]

Docs LGTM.

[jasnell]

hmm... not really a fan of introducing new environment variables at this point. Why would this not be an API option?

[mcollina]

LGTM

[BridgeAR]

@jasnell assert has no options so far. I thought about three different implementations: a) using the environment variable. b) Refactoring assert in a backwards compatible way to a class and allowing defaults. c) adding options to the function calls.

a) is the most straight forward
b) is more work but it would probably be the nicest API
c) adding options directly to the API call prevents different possibilities. The API is also not that nice to use.

I am against c) because I would like to go into a different direction there. I could rewrite it to b) if you would rather have that. In that case the assert as is currently exist would just be a instance of the class.

[jasnell]

I'd almost prefer option B. Yes, it's more work, but environment variables are not really a great approach, even if simple.

[joyeecheung]

Can't really review the code as I am not really familiar with assert.js now, but I don't mind the environment variable as options in assert makes it more complicated. In general +1 to the idea

[addaleax]

@BridgeAR assert should definitely have options, it already depends on things it should not depend on (like color properties of streams where data may or may not end up). This PR, in its current form, makes things worse.

I’m not sure if I understand option b) correctly, but if it’s something like new assert.Assert({ colors: <bool>, strict: <bool>, fullErrors: <bool>, … }), I’m on board.

[BridgeAR]

@addaleax yes, you understood correct :-) I am planning to open a new PR with that change soon and to close this one at that point.

[Trott]

Red X'ing this just to make sure no one lands it. My understanding is that a new PR will be opened that introduces an option rather than using an environment variable like this PR does.

[BridgeAR]

Closing for now. I'll open a new PR when I get to it.