`chai.should()` breaks error serialization

[voxpelli]

Version

18.17.0

Platform

MacOS

Subsystem

test

What steps will reproduce the bug?

In a test file like:

import { test } from "node:test";

test("failing test", () => {
  throw new Error('The failure');
});

Run that with node --test and you'll get:

✖ failing test (0.59475ms)
  Error: The failure
      at TestContext.<anonymous>

Add in chai.should():

import { test } from "node:test";
import chai from 'chai';

chai.should();

test("failing test", () => {
  throw new Error('The failure');
});

And you instead get:

[Error [ERR_TEST_FAILURE]: The failure] {
    [cause]: Error: The failure
        at TestContext.<anonymous>

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

All ERR_TEST_FAILURE events given in the data.details.error of a TestStream test:fail should have an err.code and should have that equal 'ERR_TEST_FAILURE', signaling that its err.cause contains the error thrown in the failing test:

node/lib/internal/test_runner/reporter/spec.js

Line 59 in 36c72c8

const err = error.code === 'ERR_TEST_FAILURE' ? error.cause : error;

What do you see instead?

In Node 18.16.1 I saw that, but in Node 18.17.0 adding chai.should() makes it so that no err.code at all appears in the errors in data.details.error in the 'test:fail' TestStream event, yet the stack trace shows that its indeed a ERR_TEST_FAILURE in there somewhere.

This causes checks like these to fail:

node/lib/internal/test_runner/reporter/spec.js

Line 59 in 36c72c8

const err = error.code === 'ERR_TEST_FAILURE' ? error.cause : error;

And eg. causes my diff of Chai errors in @voxpelli/node-test-pretty-reporter to no longer work (unless I do workarounds and check for the presence of the 'ERR_TEST_FAILURE' string in the stack).

Additional information

This is a follow up to #48900 (comment) and is probably caused by the #47867 that @MoLow mentioned in there and which fixed that issue.

[MoLow]

can you alaborate more?
I run this piece of code, which does nothing, and throws no error:

const chai = require("chai");
chai.should();

also - in your example you call this outside of a test - is that intended?

[voxpelli]

@MoLow The mere act of chai.should() makes it so that no err.code is found on ERR_TEST_FAILURE errors in the reporters, including the built in one.

This is the minimal reproduction of the issue I saw here but you couldn’t reproduce: #48900 (comment)

After narrowing it down I realized that the mere activation of the “should”-style of Chai caused it (the one that enables one to write foo.should.equal('bar'))

[cjihrig]

This appears to be related to Node's error serialization, and not specific to the test runner. It might be related to 78972d4696, which is new in 18.17.0, but I have not bisected.

The bug shows up with --test, but not if the file is run directly. When --test is used and the test fails, it serializes the error (via serializeError()) in order to report it. serializeError() calls TryGetAllProperties(), which iterates over everything on the error's prototype.

chai.should() creates an Object.prototype.should getter. Node's error serialization triggers that getter in TryGetAllProperties(). That getter introduces a Proxy on the Object prototype, which breaks the serialization.

Here is a more minimal repro (note you must start Node with --expose-internals):

'use strict';
const {
  deserializeError,
  serializeError
} = require('internal/error_serdes');

// Comment out the following line to see the difference.
Object.prototype.boom = new Proxy({}, {});

const err = new Error('message');
err.code = 123;

const serialized = serializeError(err);
const deserialized = deserializeError(serialized);

console.log(deserialized.code);

I would say that chai should not be doing what it's doing, but Node could also handle it better.

[BridgeAR]

I am going to close this since the prototype of Object is manipulated and that is not a good idea. It could have side effects everywhere.