toEqual/toStrictEqual does not check custom Error properties

[steffanek]

Describe the bug

Hi, I'm using the effect library to construct class instances, and I encounter the following issues, where the 2 assertions below pass (which is wrong):

import { Schema } from "@effect/schema";
import { Data } from "effect";
import { describe, expect, it } from "vitest";

class DataTaggedError extends Data.TaggedError("DataTaggedError")<{
  msg: string;
}> {}

class SchemaTaggedError extends Schema.TaggedError<SchemaTaggedError>()(
  "SchemaTaggedError",
  {
    msg: Schema.string,
  }
) {}

describe("testing", () => {
  it("debugging", () => {
    //Data.TaggedError
    expect({
      error: new DataTaggedError({ msg: "a" }),
    }).toStrictEqual({
      error: new DataTaggedError({ msg: "b" }),
    });

    //Schema.TaggedError
    expect({
      error: new SchemaTaggedError({ msg: "a" }),
    }).toStrictEqual({
      error: new SchemaTaggedError({ msg: "b" }),
    });
  });
});

I've initially raised an issue on the effect, but it result that by comparing with the native assert it works as expected:

import { Schema } from "@effect/schema";
import { Data } from "effect";
import * as assert from "node:assert";

class SchemaTaggedError extends Schema.TaggedError<SchemaTaggedError>()(
  "SchemaTaggedError",
  {
    msg: Schema.string,
  }
) {}

class DataTaggedError extends Data.TaggedError("DataTaggedError")<{
  msg: string;
}> {}

// no error
assert.deepStrictEqual(
  { error: new DataTaggedError({ msg: "a" }) },
  { error: new DataTaggedError({ msg: "a" }) }
);

// error report properly
assert.deepStrictEqual(
  { error: new DataTaggedError({ msg: "a" }) },
  { error: new DataTaggedError({ msg: "b" }) }
);

// no error
assert.deepStrictEqual(
  { error: new SchemaTaggedError({ msg: "a" }) },
  { error: new SchemaTaggedError({ msg: "a" }) }
);

// error report properly
assert.deepStrictEqual(
  { error: new SchemaTaggedError({ msg: "a" }) },
  { error: new SchemaTaggedError({ msg: "b" }) }
);

Reproduction

reproduction on StackBlitz with the code above

System Info

System:
    OS: macOS 12.2.1
    CPU: (8) arm64 Apple M1 Pro
    Memory: 476.78 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 20.3.0 - ~/.volta/tools/image/node/20.3.0/bin/node
    Yarn: 1.22.19 - ~/.volta/bin/yarn
    npm: 9.6.7 - ~/.volta/tools/image/node/20.3.0/bin/npm
    pnpm: 8.3.1 - ~/.volta/bin/pnpm
  Browsers:
    Chrome: 121.0.6167.184
    Safari: 15.3
  npmPackages:
    vitest: ^1.2.2 => 1.2.2

Used Package Manager

pnpm

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[fenghan34]

This happened as both DataTaggedError and SchemaTaggedError are extended from Error, and for Error objects, now Vitest will only checks the message prop.

vitest/packages/expect/src/jest-utils.ts

Line 98 in c692f76

return a.message === b.message

Perhaps Vitest need to do more checks when both message props are undefined, I'm willing to make a PR to fix this.

[hi-ogawa]

I think jest still has the same code. There's one similar issue and even PR too, but they got closed as they didn't receive attention:

• [Bug]: toThrow and toStrictEqual matchers compare only message field of errors jestjs/jest#13604
• fix: Strict equality for errors and throws custom classes jestjs/jest#13607

If there were no back-compat or jest-compat concern, then personally it makes sense to do the same treatment as NodeJS's assertion https://nodejs.org/api/assert.html#comparison-details_1, namely these two:

• Only enumerable "own" properties are considered.
• Error names and messages are always compared, even if these are not enumerable properties.

But in practice, changing this condition might have some larger implication.

---

I haven't tried but it might be possible to implement stricter Error instance equality on user land by expect.addEqualityTesters https://vitest.dev/api/expect.html#expect-addequalitytesters.

[steffanek]

This happened as both DataTaggedError and SchemaTaggedError are extended from Error, and for Error objects, now Vitest will only checks the message prop.

vitest/packages/expect/src/jest-utils.ts

Line 98 in c692f76

return a.message === b.message

Perhaps Vitest need to do more checks when both message props are undefined, I'm willing to make a PR to fix this.

@fenghan34 even they are extending from Error, I would expect from the toStrictEqual to check if all key values matches. Does that make sense?

[hi-ogawa]

@steffanek Thanks for raising an issue btw.

The current behavior doesn't seem intuitive for me as well, but the code we're seeing here is originally from Jest https://github.com/jestjs/jest/blob/e6d60cb1ab04a2d691dcbf025ed53d7a07327889/packages/expect-utils/src/jasmineUtils.ts#L86-L88 and the comment says it's based on underscore, so it could be like more than 10 years old.

What I just wanted to add is that, there could be some reason people have been adopting this behavior, so I think it's worth spending time surveying some history and more comparison.

Like you've already shown, node:assert is different (and personally it makes sense to me). But, we might want to also check others like lodash's isEqual, other test frameworks and assertion libraries.

In terms of documentation, there's at least a warning about this for toEqual (and internally toStrictEqual doesn't treat Error differently):

• https://vitest.dev/api/expect.html#toequal
• https://jestjs.io/docs/expect#toequalvalue

---

Also here is a repro to show the current behavior without effect:

https://stackblitz.com/edit/vitest-dev-vitest-jzxduv?file=test%2Fsimple.test.ts

Show code

import { expect, it, assert } from 'vitest';
import nodeAssert from 'node:assert';

class MyError extends Error {
  constructor(message: string, public custom: string) {
    super(message);
  }
}

class YourError extends Error {
  constructor(message: string, public custom: string) {
    super(message);
  }
}

it('custom', () => {
  const e1 = new MyError('hi', 'a');
  const e2 = new MyError('hi', 'b');
  expect(e1).toEqual(e2);
  expect(e1).toStrictEqual(e2);
  assert.deepEqual(e1, e2);
  nodeAssert.notDeepStrictEqual(e1, e2);
});

it('message', () => {
  const e1 = new MyError('hi', 'a');
  const e2 = new YourError('hello', 'a');
  expect(e1).not.toEqual(e2);
  expect(e1).not.toStrictEqual(e2);
  assert.notDeepEqual(e1, e2);
  nodeAssert.notDeepStrictEqual(e1, e2);
});

it('class', () => {
  const e1 = new MyError('hello', 'a');
  const e2 = new YourError('hello', 'b');
  expect(e1).toEqual(e2);
  expect(e1).not.toStrictEqual(e2);
  assert.deepEqual(e1, e2);
  nodeAssert.notDeepStrictEqual(e1, e2);
});

---

Next I'll see if I can do something with expect.addEqualityTesters for the time being https://vitest.dev/api/expect.html#expect-addequalitytesters

[hi-ogawa]

I explored expect.addEqualityTesters based approach and I think this should work for your use case for the time being:
https://stackblitz.com/edit/vitest-dev-vitest-944rfr?file=test%2Fbug.spec.ts

On Vitest side, probably what we can do for now is to just put more warning on documentation #5253
Maybe if this behavior is desired so much, then we could introduce some opt-in flag to automatically enable this custom equality tester, but for that, we'll probably need more discussion based on use cases.

[fenghan34]

even they are extending from Error, I would expect from the toStrictEqual to check if all key values matches. Does that make sense?

@steffanek Yes, It makes sense to me to be honest. And I think @hi-ogawa has provided the appropriate solution at this time.

[steffanek]

Thanks @hi-ogawa and @fenghan34 for your quick response and solution.
Another solution would be to simply import * as assert from "node:assert" and use it directly in test files, whenever we want to check a deepStrictEqual in case of any instances. Do you guys see some downsides? What can comes to my mind is: performance (comparing to expect.addEqualityTesters) or wrong testing report?

[sheremet-va]

Another solution would be to simply import * as assert from "node:assert" and use it directly in test files

Just a note, Vitest supports extended assert API: https://vitest.dev/api/assert.html

[steffanek]

Another solution would be to simply import * as assert from "node:assert" and use it directly in test files

Just a note, Vitest supports extended assert API: https://vitest.dev/api/assert.html

@sheremet-va assert.deepStrictEqualfrom vitest does not solve this issue.

[hi-ogawa]

Another solution would be to simply import * as assert from "node:assert" and use it directly in test files, whenever we want to check a deepStrictEqual in case of any instances. Do you guys see some downsides? What can comes to my mind is: performance (comparing to expect.addEqualityTesters) or wrong testing report?

@steffanek Using node:assert should work fine, but the downside is simply that it's not a part of expect API, which would mean for example:

• it might be prone to forget switching to assert.deepStrictEqual(...) if you are using expect(...).toStrictEqual(...) around the code.
• some expect API works in a "stateful" way, such as expect.hasAssertions doesn't take into assert.deepStrictEqual account when counting the number of assertions.
• there's no expect.soft version of assert.deepStrictEqual

Regarding performance, I would guess node:assert would be faster since expect/toEqual does more stuff underneath. But I don't think such raw performance difference would practically matter for normal usages.

[steffanek]

Makes sense @hi-ogawa thanks for your contribution. Let's see if this behavior is really desired, to enable it by default.

[steffanek]

Hi @hi-ogawa it seems that I do not need anymore to use the custom matcher that you've initially provided.
However, now I got the following issue (see below the last test where I compared two errors values inside an Exit data type :

import { it, expect } from "vitest";
import { Schema } from "@effect/schema";
import { Exit } from "effect";

class SchemaTaggedError extends Schema.TaggedError<SchemaTaggedError>()(
  "SchemaTaggedError",
  {
    msg: Schema.String,
    other: Schema.String,
  }
) {}

class SchemaTaggedClass extends Schema.TaggedClass<SchemaTaggedClass>()(
  "SchemaTaggedClass",
  {
    msg: Schema.String,
    other: Schema.String,
  }
) {}

it("Error type", () => {
  expect(new SchemaTaggedError({ msg: "a", other: "other" })).toStrictEqual(
    new SchemaTaggedError({ msg: "a", other: "other" })
  );
});

it("Exit with SchemaTaggedClass", () => {
  expect(
    Exit.fail(new SchemaTaggedClass({ msg: "a", other: "other" }))
  ).toStrictEqual(
    Exit.fail(new SchemaTaggedClass({ msg: "a", other: "other" }))
  );
});

//This should not pass..
it("Exit with SchemaTaggedError", () => {
  expect(
    Exit.fail(new SchemaTaggedError({ msg: "a", other: "other" }))
  ).toStrictEqual(
    Exit.fail(new SchemaTaggedError({ msg: "b", other: "other" }))
  );
});