Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chai.should() breaks error serialization #48918

Open
voxpelli opened this issue Jul 25, 2023 · 3 comments
Open

chai.should() breaks error serialization #48918

voxpelli opened this issue Jul 25, 2023 · 3 comments

Comments

@voxpelli
Copy link

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:

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:

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
Copy link
Member

MoLow commented Jul 25, 2023

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
Copy link
Author

voxpelli commented Jul 25, 2023

@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
Copy link
Contributor

cjihrig commented Jul 25, 2023

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.

@cjihrig cjihrig removed the test_runner Issues and PRs related to the test runner subsystem. label Jul 25, 2023
@cjihrig cjihrig changed the title 18.17.0 test runner gives reporters no error metadata with chai.should() chai.should() breaks error serialization Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants