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

snapshot stored value will be changed after assert_match #99

Open
lucemia opened this issue Oct 17, 2019 · 3 comments
Open

snapshot stored value will be changed after assert_match #99

lucemia opened this issue Oct 17, 2019 · 3 comments

Comments

@lucemia
Copy link
Contributor

lucemia commented Oct 17, 2019

e.g.

test case

def test_f(snapshot):
    x = {}
    snapshot.assert_match(x)  # should be {}

    x["a"] = 1
    snapshot.assert_match(x)

snapshot

snapshots['test_f 1'] = {
    'a': 1  # <-- wrong value
}

snapshots['test_f 2'] = {
    'a': 1
}
@lucemia lucemia changed the title snapshot will be changed after assert_match snapshot stored value will be changed after assert_match Oct 17, 2019
@paulmelnikow
Copy link
Collaborator

Would welcome a PR to fix this! Not sure whether that will be easy or not.

@medmunds
Copy link
Collaborator

This looks like it will be non-trivial to fix.

The problem is that during updating, SnapshotTest.assert_match just stores a simple reference to the value it's passed. The value isn't actually serialized into a snapshot until later (after all tests in a particular module). So if anything modifies that value in between, the snapshot will be written with the wrong value.

Couple of options:

  • Store a deep copy of the value in assert_match. (This seems simple, but there are a lot of corner cases, and I'm not even sure it's possible for all the value types supported.)
  • Serialize the snapshot immediately in SnapshotTest.store (or BaseFormatter.store). I think this is the right fix, but it involves a substantial reworking of the "formatters" code, and how they are later combined into a single snapshot file (merging imports, etc.)

@paulmelnikow
Copy link
Collaborator

Thanks for looking into this.

Serialize the snapshot immediately in SnapshotTest.store (or BaseFormatter.store). I think this is the right fix, but it involves a substantial reworking of the "formatters" code, and how they are later combined into a single snapshot file (merging imports, etc.)

Agree that this would be the way to go.

15r10nk added a commit to 15r10nk/inline-snapshot that referenced this issue Jul 12, 2023
BREAKING CHANGE: values have to be copyable with `copy.deepcopy`

This is a behaviour which is already expected from other libraries.

syrusakbary/snapshottest#99
15r10nk added a commit to 15r10nk/inline-snapshot that referenced this issue Jul 12, 2023
BREAKING CHANGE: values have to be copyable with `copy.deepcopy`

This is a behaviour which is already expected from other libraries.

syrusakbary/snapshottest#99
15r10nk added a commit to 15r10nk/inline-snapshot that referenced this issue Jul 12, 2023
BREAKING CHANGE: values have to be copyable with `copy.deepcopy`

This is a behaviour which is already expected from other libraries.

syrusakbary/snapshottest#99
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants