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.

Sign up for GitHub

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

fix: check current session's pending-write queue when recalling snapshots (e.g. diffing) #927

Merged
merged 8 commits into from
Jan 13, 2025
Merged

fix: check current session's pending-write queue when recalling snapshots (e.g. diffing) #927

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dbf395a
fix: check current session's pending-write queue when recalling snaps…
huonw Dec 22, 2024
0fb10fd
Make PyTestLocation hashable
huonw Jan 13, 2025
0f694ca
Explicitly set methodname to None for doctests
huonw Jan 13, 2025
c577ba6
Queue writes with a dict for O(1) look-ups
huonw Jan 13, 2025
55c4995
Use type aliases
huonw Jan 13, 2025
9a3c9f0
return both keys from _snapshot_write_queue_key
huonw Jan 13, 2025
817de52
Use a defaultdict
huonw Jan 13, 2025
a86f5d8
Update comments
huonw Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/syrupy/assertion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,11 +377,7 @@ def _recall_data(
) -> Tuple[Optional["SerializableData"], bool]:
try:
return (
self.extension.read_snapshot(
test_location=self.test_location,
index=index,
session_id=str(id(self.session)),
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 note that #606 added this session_id parameter, which I imagine was meant to help with circumstances like this, but it seems like it ends up ignored/unused in practice?

And, in practice, I think it may be quite hard to use, since the relevant cached data is held in memory in the session itself (i.e. the extension _read_snapshot_data_from_location method calls don't really have access to it at all).

Copy link
Collaborator

@noahnu noahnu Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The session_id is used as the cache_key argument in Amber's __cacheable_read_snapshot method. Although it's not used in the function, it's used by the lru_cache decorator (which caches based on the kwargs I believe). So it essentially invalidates the cache

),
self.session.recall_snapshot(self.extension, self.test_location, index),
False,
)
except SnapshotDoesNotExist:
Expand Down
37 changes: 33 additions & 4 deletions src/syrupy/session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,25 @@ class SnapshotSession:
List[Tuple["SerializedData", "PyTestLocation", "SnapshotIndex"]],
] = field(default_factory=dict)

def queue_snapshot_write(
def _snapshot_write_queue_key(
self,
extension: "AbstractSyrupyExtension",
test_location: "PyTestLocation",
data: "SerializedData",
index: "SnapshotIndex",
) -> None:
) -> Tuple[Type["AbstractSyrupyExtension"], str]:
snapshot_location = extension.get_location(
test_location=test_location, index=index
)
key = (extension.__class__, snapshot_location)
return (extension.__class__, snapshot_location)

def queue_snapshot_write(
self,
extension: "AbstractSyrupyExtension",
test_location: "PyTestLocation",
data: "SerializedData",
index: "SnapshotIndex",
) -> None:
key = self._snapshot_write_queue_key(extension, test_location, index)
queue = self._queued_snapshot_writes.get(key, [])
queue.append((data, test_location, index))
self._queued_snapshot_writes[key] = queue
Expand All @@ -93,6 +101,27 @@ def flush_snapshot_write_queue(self) -> None:
)
self._queued_snapshot_writes = {}

def recall_snapshot(
self,
extension: "AbstractSyrupyExtension",
test_location: "PyTestLocation",
index: "SnapshotIndex",
) -> Optional["SerializedData"]:
"""Find the current value of the snapshot, for this session, either a pending write or the actual snapshot."""

key = self._snapshot_write_queue_key(extension, test_location, index)
queue = self._queued_snapshot_writes.get(key)
if queue:
# find the last (i.e. most recent) write to this index/location in the queue:
for queue_data, queue_test_location, queue_index in reversed(queue):
if queue_index == index and queue_test_location == test_location:
return queue_data
Copy link
Contributor Author

@huonw huonw Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This loops is a potential performance issue, but I suspect it is okay. In particular: it scales like O(length of queue), i.e. number of pending writes.

This function is called twice in SnapshotAssertion._assert:

If every assertion in a ambr file needs an update, this leads to quadratic O(number of assertions in that file^2) behaviour:

  • the first assert ... == snapshot has an empty queue, and adds an item
  • the second checks one element of the queue (doesn't match), and adds an item
  • the third checks two elements of the queue (don't match) and adds an item
  • ...
  • the Nth checks N - 1 elements of the queue (all don't match) and adds an item

i.e. N assertions ends up iterating over $0 + 1 + 2 + ... + (N - 1) = \frac{(N - 1)(N - 2)}{2} = O(N^2)$ elements of queue.

I think this is probably okay:

  • this scales per snapshot location, not across the full testing session, since each queue is just for one key, which incorporates the snapshot location (i.e. if one has 1000 assertions, spread evenly across 100 test file (i.e. 10 assertions in each file), this is scaling with N = 10, not N = 1000).
  • how often does one have to completely re-write a whole file?
  • the work here is individually cheap (i.e. the constant factor is low)

Copy link
Collaborator

@noahnu noahnu Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There appears to be a slowdown: #927 (comment)

I ran inv benchmark locally as well. Results for this branch:

------------------------------------------------------------------------------ benchmark: 3 tests ------------------------------------------------------------------------------
Name (time in s)         Min               Max              Mean            StdDev            Median               IQR            Outliers     OPS            Rounds  Iterations
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_1000x_reads      1.0756 (1.0)      1.2278 (1.0)      1.1648 (1.0)      0.0630 (4.10)     1.1974 (1.0)      0.0948 (3.49)          2;0  0.8585 (1.0)           5           1
test_standard         1.1904 (1.11)     1.3294 (1.08)     1.2822 (1.10)     0.0534 (3.48)     1.2960 (1.08)     0.0439 (1.62)          1;1  0.7799 (0.91)          5           1
test_1000x_writes     1.3974 (1.30)     1.4278 (1.16)     1.4089 (1.21)     0.0153 (1.0)      1.3986 (1.17)     0.0272 (1.0)           1;0  0.7098 (0.83)          5           1
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

and "main":

------------------------------------------------------------------------------ benchmark: 3 tests ------------------------------------------------------------------------------
Name (time in s)         Min               Max              Mean            StdDev            Median               IQR            Outliers     OPS            Rounds  Iterations
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_1000x_reads      1.0264 (1.0)      1.1148 (1.0)      1.0855 (1.0)      0.0362 (1.0)      1.1006 (1.0)      0.0474 (1.0)           1;0  0.9212 (1.0)           5           1
test_standard         1.0904 (1.06)     1.1974 (1.07)     1.1461 (1.06)     0.0477 (1.32)     1.1414 (1.04)     0.0876 (1.85)          2;0  0.8725 (0.95)          5           1
test_1000x_writes     1.0937 (1.07)     1.2011 (1.08)     1.1406 (1.05)     0.0405 (1.12)     1.1313 (1.03)     0.0534 (1.13)          2;0  0.8767 (0.95)          5           1
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

The 1000x writes test performs 0.8767 operations per second on main but only 0.7098 operations per second with this change. Roughly 20% slowdown.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this scales per snapshot location,

This comes up when users are leveraging test parameterization over data sets. i.e. the benchmark case of parameterizing over 1000 test cases isn't too unrealistic from how I've seen syrupy used in the past.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, the benchmarking is broken in CI for contributors (something to do with permissions in the github workflow I need to fix), however you should be able to run it locally with inv benchmark.

Copy link
Contributor Author

@huonw huonw Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Updated. See #927 (comment)

Thanks for proving my "I think" wasn't accurate 😄


# no queue, or no matching write, so just read the snapshot directly:
return extension.read_snapshot(
test_location=test_location, index=index, session_id=str(id(self))
)

@property
def update_snapshots(self) -> bool:
return bool(self.pytest_session.config.option.update_snapshots)
Expand Down
56 changes: 56 additions & 0 deletions tests/integration/test_snapshot_diff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import pytest

_TEST = """
def test_foo(snapshot):
assert {**base} == snapshot(name="a")
assert {**base, **extra} == snapshot(name="b", diff="a")
"""


def _make_file(testdir, base, extra):
testdir.makepyfile(
test_file="\n\n".join([f"base = {base!r}", f"extra = {extra!r}", _TEST])
)


def _run_test(testdir, base, extra, expected_update_lines):
_make_file(testdir, base=base, extra=extra)

# Run with --snapshot-update, to generate/update snapshots:
result = testdir.runpytest(
"-v",
"--snapshot-update",
)
result.stdout.re_match_lines((expected_update_lines,))
assert result.ret == 0

# Run without --snapshot-update, to validate the snapshots are actually up-to-date
result = testdir.runpytest("-v")
result.stdout.re_match_lines((r"2 snapshots passed\.",))
assert result.ret == 0


def test_diff_lifecycle(testdir) -> pytest.Testdir:
# first: create both snapshots completely from scratch
_run_test(
testdir,
base={"A": 1},
extra={"X": 10},
expected_update_lines=r"2 snapshots generated\.",
)

# second: edit the base data, to change the data for both snapshots (only changes the serialized output for the base snapshot `a`).
_run_test(
testdir,
base={"A": 1, "B": 2},
extra={"X": 10},
expected_update_lines=r"1 snapshot passed. 1 snapshot updated\.",
)

# third: edit just the extra data (only changes the serialized output for the diff snapshot `b`)
_run_test(
testdir,
base={"A": 1, "B": 2},
extra={"X": 10, "Y": 20},
expected_update_lines=r"1 snapshot passed. 1 snapshot updated\.",
)
Loading