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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Omit non-enumerable, symbolic members in snapshots #7443

Closed
austinalameda opened this issue Nov 30, 2018 · 7 comments 路 Fixed by #7448
Closed

Omit non-enumerable, symbolic members in snapshots #7443

austinalameda opened this issue Nov 30, 2018 · 7 comments 路 Fixed by #7448

Comments

@austinalameda
Copy link

馃悰 Bug Report

When upgrading from MobX 4.3.0 to 5.6.0, new snapshot information is created using jest

This information is largely extraneous, and when added typically triples the size of the snapshot file (best case) and sometimes grows file by 10x (worst case)

exports[`Size Picker Snapshot not clicked 1`] = `
<MuiThemeProvider>
  <SizePicker
    store={
      SizePickerStore {
        "defaultValue": 8,
        "idPrefix": "size-picker",
        "internalId": "1",
        "isExpanded": false,
        "isMousePressed": false,
        "label": undefined,
        "onChange": Event {
          "listeners": Array [],
          "publishRecursionCount": 0,
        },
        "onHideShow": [Function],
        "onKeyDown": [Function],
        "onMouseDown": [Function],
        "onMouseUp": [Function],
        "value": 8,
+      Symbol(mobx did run lazy initializers): true,
+      Symbol(mobx administration): ObservableObjectAdministration$$1 {
+      "defaultEnhancer": [Function],
+      "keysAtom": Atom$$1 {
+        "diffValue": 0,
+        "isBeingObserved": false,
+        "isPendingUnobservation": false,
+        "lastAccessedBy": 0,
+        "lowestObserverState": 2,
+        "name": "SizePickerStore@26.keys",
+        "observers": Set {},
          },
          "name": "SizePickerStore@26",
          "target": [Circular],
          "values": Map {
            "value" => 8,
            "isExpanded" => false,
            "label" => undefined,
          },
        },
      }
    }
  >

To Reproduce

Steps to reproduce the behavior: Using mobx and mobx-react, create an @observer react component that receives props including an @observable object. Write a test file that mounts this component and takes a snapshot

Expected behavior

according to @mweststrate of MobX:
the problem is that it should ignore non-enumerable (symbolic) members. It was fixed a while ago for toEqual, but not yet for snapshots

link to mobx issue

Run npx envinfo --preset jest

System:
    OS: Windows 10
    CPU: (8) x64 Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz
  Binaries:
    Node: 10.13.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.12.3 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 5.0.4 - C:\Program Files\nodejs\npm.CMD
@mweststrate
Copy link
Contributor

For some background, at first glance it seems that snapshots are accidentally taking non-enumerable, symbolic members into account. This was originally the case for toEquals as well, but has been addressed in #6398. I think it would make sense that snapshots follows the same semantics.

I am willing to attempt a PR to achieve the mentioned behavior

@SimenB
Copy link
Member

SimenB commented Dec 1, 2018

It might help the large jsdom snapshots as well...

/cc @pedrottimark @rickhanlonii @thymikee thoughts?

@mweststrate
Copy link
Contributor

Suggested alternative title for this issue: Omit non-enumerable, symbolic members in snapshots

@thymikee
Copy link
Collaborator

thymikee commented Dec 1, 2018

Since we've changed this behavior for toEqual, I think it makes sense to adjust snapshots this way as well.
EDIT: Another breaking change as well 馃槄

@rickhanlonii
Copy link
Member

@austinalameda thanks for reporting and thanks for dropping in with the info @mweststrate

If we're going to do it, let's do it now before the next major

Another option (either in the short term or long term) may be to add a custom serializer

@pedrottimark
Copy link
Contributor

Yes to making pretty-format consistent with toEqual matcher. It will help with jsdom too.

I will make pull request.

@austinalameda austinalameda changed the title Mobx observable in jest snapshot too large Omit non-enumerable, symbolic members in snapshots Dec 4, 2018
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants