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

Producing unstable snapshots #5681

Closed
6 tasks done
dubzzz opened this issue May 7, 2024 · 7 comments · Fixed by #5724
Closed
6 tasks done

Producing unstable snapshots #5681

dubzzz opened this issue May 7, 2024 · 7 comments · Fixed by #5724
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority) upstream

Comments

@dubzzz
Copy link
Contributor

dubzzz commented May 7, 2024

Describe the bug

The name of the test printed in the snapshot file may include wrongly-formed characters. On windows, it leads Vitest to consider the snapshot as obsolete and recreate a new one for each run.

Where do the bug comes from? -> loupe

The bug is visible in Vitest but comes from loupe and has been reported on their side too at chaijs/loupe#78.

In Vitest the call to loupe responsible to the problem is in @vitest/utils/dist/chunk-display.js within objDisplay. Once fixed on loupe's side I can probably help and come with some dedicated tests covering that issue to avoid it to come back.

Temporary fix

While waiting for a fix on loupe's side, I can propose a fix on your side that will drop/replace surrogates not being in pairs? What do you think?

Reproduction

Consider the following example:

import { expect, test } from "vitest";

test.each([{ value: "\u{1f431}".repeat(50) }])(
  "print the value properly for value=$value",
  ({ value }) => {
    expect(value).toMatchSnapshot();
  }
);

It leads to the snapshot-ed value:

exports[`print the value properly for value='🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱�…' 1`] = `"🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱🐱"`;

This snapshot name is invalid as it comes with a split-cat or more precisely a split-surrogate-pair which is not an existing character outside of JavaScript.

System Info

System:
    OS: Windows 11 10.0.22621
    CPU: (4) x64 Intel(R) Core(TM) i5-3570K CPU @ 3.40GHz
    Memory: 1.50 GB / 7.89 GB
  Binaries:
    Node: 20.8.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.21 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 10.2.3 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Chromium (123.0.2420.97)
    Internet Explorer: 11.0.22621.1
  npmPackages:
    vitest: ^1.5.3 => 1.5.3

Used Package Manager

npm

Validations

@dubzzz
Copy link
Contributor Author

dubzzz commented May 7, 2024

The issue can easily be detected (needs possibly several retries) with the following property based test:

import fc from 'fast-check'

describe('objDisplay', () => {
  test('Only produce valid strings with objDisplay', () => {
    // NOTE: The instancitation of fc.anything(...) should probably be made shorter, today it does not provide any unicode-aware
    // built-in version and thus require users to be verbose... Ideally, it should be as simple as `fc.anything({withUnicode: true})`
    fc.assert(fc.property(fc.anything({ key: fc.fullUnicodeString(), values: [
      fc.boolean(),
      fc.maxSafeInteger(),
      fc.double(),
      fc.fullUnicodeString(),
      fc.oneof(fc.fullUnicodeString(), fc.constant(null), fc.constant(undefined)),
    ] }), fc.integer({ min: 2 }), (value, truncate) => {
      expect(() => encodeURI(objDisplay(value, { truncate }))).not.toThrow()
    }))
  })
})

// FAIL  |threads| test/utils.spec.ts > objDisplay > Only produce valid strings with objDisplay
// Error: Property failed after 74 tests
// { seed: -1289013075, path: "73:1:1:2:2:2:4:3:2:2:3:2:2:2:3:2:2:2:6:5:5:7:5:6:6:6:9:7:6:7:0", endOnFailure: true }
// Counterexample: ["𐀀 ",4]
// Shrunk 30 time(s)
// Got AssertionError: expected [Function] to not throw an error but 'URIError: URI malformed' was thrown

If adding tests (waiting your go), I'll probably not use property based technique for now to avoid pulling something additional to your dev-deps. But I wanted to let you know that it could help to detect such issues in an automatic fashion. So I can work on adding some if you feel it might be interested (Jest found many problems thanks to fast-check).

@sheremet-va sheremet-va added upstream p2-edge-case Bug, but has workaround or limited in scope (priority) and removed pending triage labels May 7, 2024
@dubzzz
Copy link
Contributor Author

dubzzz commented May 7, 2024

@sheremet-va I'm working on a fix on loupe

@dubzzz
Copy link
Contributor Author

dubzzz commented May 7, 2024

I just opened a PR on loupe's side (see chaijs/loupe#79). If merged the fix will change the behavior of inspect in the broken cases only. No impact expected on Vitest side except fixing the bug.

Once merged on their side (if merged), I'll try to propose some tests on Vitest's side if you're ok with it.

@dubzzz
Copy link
Contributor Author

dubzzz commented May 11, 2024

It should be fixed with the next version of loupe. My fix has just been merged on their side. Let me know if you want me to cover the issue with tests so that it gets caught next time

@sheremet-va
Copy link
Member

It should be fixed with the next version of loupe. My fix has just been merged on their side. Let me know if you want me to cover the issue with tests so that it gets caught next time

It would be nice to have a test case for this in Vitest repo. You can add your tests somewhere to test/core, but I would like to keep it simple without using fast-check.

@dubzzz
Copy link
Contributor Author

dubzzz commented May 14, 2024

It would be nice to have a test case for this in Vitest repo. You can add your tests somewhere to test/core, but I would like to keep it simple without using fast-check.

Will do!

dubzzz added a commit to dubzzz/vitest that referenced this issue May 14, 2024
dubzzz added a commit to dubzzz/vitest that referenced this issue May 14, 2024
dubzzz added a commit to dubzzz/vitest that referenced this issue May 14, 2024
@dubzzz
Copy link
Contributor Author

dubzzz commented May 14, 2024

I just opened a PR fixing the issue by pulling the latest version of loupe. It also comes with relevant tests.

See #5724

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority) upstream
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants