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

feat: pretty print diffs coming from cause #5660

Merged
merged 5 commits into from
May 6, 2024

Conversation

dubzzz
Copy link
Contributor

@dubzzz dubzzz commented May 3, 2024

Description

This Pull Request adds support for diffs coming nested errors. In the context of Error with cause, if the cause is an Error coming with a diff, the current version of Vitest does not render it in a diff fashion. It tends to make understanding such nested errors complex. The proposal implemented by this PR is to display the diff attached to causes.

Let's consider the following snippet:

import { test, expect } from "vitest";

const value1 = { a: 1, b: 2, c: 3, e: 4, f: 5, g: 6, h: 7, i: 8, j: 9, k: 10 };
const value2 = { a: 1, b: 2, c: 3, e: 4, f: 5, g: 6, h: 7, i: 88, j: 9, k: 10 };

test("rethrown expect as cause", () => {
  try {
    expect(value1).toEqual(value2);
  } catch (err) {
    throw new Error("failed!", { cause: err });
  }
});
Before After
image image

Why is rewrapping an Error within a cause a pattern that should be considered? Property Based Testing and Fuzzing frameworks usually rewrap thrown exceptions into their own exception. When leveraging Error with cause mechanisms they may expose the source Error as a cause of their own Error.

Example of fuzzing with fast-check

Considered code:

import { test, expect } from "vitest";
import fc from "fast-check";

fc.configureGlobal({ errorWithCause: true });
const value1 = { a: 1, b: 2, c: 3, e: 4, f: 5, g: 6, h: 7, i: 8, j: 9, k: 10 };

test("fuzz", () => {
  fc.assert(
    fc.property(fc.nat(), (i) => {
      expect(value1).toEqual({ ...value1, i });
    })
  );
});
Before After
image image

More on fast-check at fast-check.dev

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

In the current version of Vitest, wrapping an error thrown by an `expect` within an other `Error` drops the ability to display proper diffs to the users.

The error thrown in the snippet below will be hard to diagnose for end user as the diff cannot be displayed:

```js
import { test, expect } from "vitest";

const value1 = { a: 1, b: 2, c: 3, e: 4, f: 5, g: 6, h: 7, i: 8, j: 9, k: 10 };
const value2 = { a: 1, b: 2, c: 3, e: 4, f: 5, g: 6, h: 7, i: 88, j: 9, k: 10 };

test("rethrown expect as cause", () => {
  try {
    expect(value1).toEqual(value2);
  } catch (err) {
    throw new Error("failed!", { cause: err });
  }
});
```

Such pattern of rewrapping errors is pretty common in Property Based Testing or fuzzing libraries as they have to run multiple times the test and append extra details to the user to help them reproducing the error (see fast-check package on npm).
Copy link

netlify bot commented May 3, 2024

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
🔨 Latest commit 890e82b
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/6635d82a57e9d5000898f574

@@ -127,6 +127,9 @@ export function processError(err: any, diffOptions?: DiffOptions) {
}
catch {}

if (typeof err.cause === 'object')
processError(err.cause, diffOptions)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Is there any reason why you don't assign the cause?
  2. If we have this recursion, then #L125-126 are not needed anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code of processError is actually performing a set of side-effects on the passed instance. By calling processError(err.cause, diffOptions), I'm actually updating err.cause in place.

Well good point for #L125-126. I'll check back, but they might not be needed anymore as the code I added will recursively sanitize the causes. By the way it will not only sanitize the first level but all the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I treated the 2. Regarding the 1, I could have done:

err.cause = processError(err.cause, diffOptions);

But I feel it might be more risky as re-assigning .cause is not needed. It may throw. As we alter it in-place during the call, there is no real need to re-assign (re-assigning to itself). I wrote not in italic as actually they are some cases that may justify the attempt to reassign as in some code paths the returned value is not self.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm ignoring the result I might just split processError into two functions: one sanitizing, the other serializing. That's said I'll go for the re-assignment idea for my attempt. Let me know what's the best option in your opinion.

@sheremet-va sheremet-va merged commit 6faf8f8 into vitest-dev:main May 6, 2024
19 checks passed
@dubzzz dubzzz deleted the pretty-diff-cause branch May 6, 2024 09:11
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

Successfully merging this pull request may close these issues.

None yet

2 participants