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

refactor: write performance improvements, api clarity #645

Merged
merged 13 commits into from
Dec 1, 2022

Conversation

noahnu
Copy link
Collaborator

@noahnu noahnu commented Nov 24, 2022

Description

This PR fixes the write perf regression in syrupy v4 prerelease. As part of the refactoring necessary to fix write perf for batch writes, I ended up simplifying some of the APIs. There are a number of breaking changes, from:

  • renaming SnapshotFossil to SnapshotCollection
  • changing properties to methods or variables
  • adding test_location as a param to a bunch of methods
  • changing instance methods to classmethods (ultimately to make write_snapshot static)
  • removal of pre/post read/write hooks (if someone has a valid use case, we can add it back -- it semantically changed when we shifted to batch writes anyway)

@codecov
Copy link

codecov bot commented Nov 24, 2022

Codecov Report

Merging #645 (ff7a2a2) into next (3325b3c) will decrease coverage by 0.00%.
The diff coverage is 99.43%.

@@            Coverage Diff             @@
##             next     #645      +/-   ##
==========================================
- Coverage   99.59%   99.58%   -0.01%     
==========================================
  Files          17       17              
  Lines        1236     1216      -20     
==========================================
- Hits         1231     1211      -20     
  Misses          5        5              

Noah Negin-Ulster added 5 commits November 30, 2022 18:20
BREAKING CHANGE: PyTestLocation.filename has been renamed to .basename
BREAKING CHANGE: Pre and post read/write hooks have been removed without replacement to make internal refactor simpler. Please open a GitHub issue if you have a use case for these hooks.
@noahnu noahnu force-pushed the fix_write_performance branch from 3f762bb to bbc7ef2 Compare December 1, 2022 20:51
BREAKING CHANGE: The term 'fossil' has been replaced by the clearer term 'collection'.
@noahnu noahnu force-pushed the fix_write_performance branch from 7413123 to 6aa2f3f Compare December 1, 2022 21:05
@noahnu noahnu changed the title wip: add TODO notes about write perf refactor: write performance improvements, api clarity Dec 1, 2022
@noahnu noahnu marked this pull request as ready for review December 1, 2022 22:48
@noahnu noahnu merged commit 1c1a157 into next Dec 1, 2022
@noahnu noahnu deleted the fix_write_performance branch December 1, 2022 22:49
noahnu added a commit that referenced this pull request Dec 30, 2022
* fix: group snapshot writes by extension class

* refactor: rename PyTestLocation.filename to .basename

BREAKING CHANGE: PyTestLocation.filename has been renamed to .basename

* refactor: add test_location kwarg to get_snapshot_name

* refactor: get_snapshot_name is now static as a classmethod

* refactor: remove pre and post read/write hooks

BREAKING CHANGE: Pre and post read/write hooks have been removed without replacement to make internal refactor simpler. Please open a GitHub issue if you have a use case for these hooks.

* refactor: rename Fossil to Collection

BREAKING CHANGE: The term 'fossil' has been replaced by the clearer term 'collection'.

* refactor: pass test_location to read_snapshot

* refactor: remove singular write_snapshot method

* refactor: dirname property to method

* refactor: pass test_location to discover_snapshots

* refactor: remove usage of self.test_location

* refactor: make write_snapshot a classmethod

* refactor: do not instantiate extension with test_location

BREAKING CHANGE: Numerous instance methods have been refactored as classmethods.
noahnu added a commit that referenced this pull request Dec 30, 2022
* fix: group snapshot writes by extension class

* refactor: rename PyTestLocation.filename to .basename

BREAKING CHANGE: PyTestLocation.filename has been renamed to .basename

* refactor: add test_location kwarg to get_snapshot_name

* refactor: get_snapshot_name is now static as a classmethod

* refactor: remove pre and post read/write hooks

BREAKING CHANGE: Pre and post read/write hooks have been removed without replacement to make internal refactor simpler. Please open a GitHub issue if you have a use case for these hooks.

* refactor: rename Fossil to Collection

BREAKING CHANGE: The term 'fossil' has been replaced by the clearer term 'collection'.

* refactor: pass test_location to read_snapshot

* refactor: remove singular write_snapshot method

* refactor: dirname property to method

* refactor: pass test_location to discover_snapshots

* refactor: remove usage of self.test_location

* refactor: make write_snapshot a classmethod

* refactor: do not instantiate extension with test_location

BREAKING CHANGE: Numerous instance methods have been refactored as classmethods.
noahnu added a commit that referenced this pull request Dec 30, 2022
* fix: group snapshot writes by extension class

* refactor: rename PyTestLocation.filename to .basename

BREAKING CHANGE: PyTestLocation.filename has been renamed to .basename

* refactor: add test_location kwarg to get_snapshot_name

* refactor: get_snapshot_name is now static as a classmethod

* refactor: remove pre and post read/write hooks

BREAKING CHANGE: Pre and post read/write hooks have been removed without replacement to make internal refactor simpler. Please open a GitHub issue if you have a use case for these hooks.

* refactor: rename Fossil to Collection

BREAKING CHANGE: The term 'fossil' has been replaced by the clearer term 'collection'.

* refactor: pass test_location to read_snapshot

* refactor: remove singular write_snapshot method

* refactor: dirname property to method

* refactor: pass test_location to discover_snapshots

* refactor: remove usage of self.test_location

* refactor: make write_snapshot a classmethod

* refactor: do not instantiate extension with test_location

BREAKING CHANGE: Numerous instance methods have been refactored as classmethods.
noahnu added a commit that referenced this pull request Dec 30, 2022
* fix: group snapshot writes by extension class

* refactor: rename PyTestLocation.filename to .basename

BREAKING CHANGE: PyTestLocation.filename has been renamed to .basename

* refactor: add test_location kwarg to get_snapshot_name

* refactor: get_snapshot_name is now static as a classmethod

* refactor: remove pre and post read/write hooks

BREAKING CHANGE: Pre and post read/write hooks have been removed without replacement to make internal refactor simpler. Please open a GitHub issue if you have a use case for these hooks.

* refactor: rename Fossil to Collection

BREAKING CHANGE: The term 'fossil' has been replaced by the clearer term 'collection'.

* refactor: pass test_location to read_snapshot

* refactor: remove singular write_snapshot method

* refactor: dirname property to method

* refactor: pass test_location to discover_snapshots

* refactor: remove usage of self.test_location

* refactor: make write_snapshot a classmethod

* refactor: do not instantiate extension with test_location

BREAKING CHANGE: Numerous instance methods have been refactored as classmethods.
tophat-opensource-bot pushed a commit that referenced this pull request Feb 2, 2023
# [4.0.0](v3.0.6...v4.0.0) (2023-02-02)

### Bug Fixes

* defer snapshot writes until end of session ([#606](#606)) ([68f1d5f](68f1d5f))
* ensure all pytest options are serializable ([#667](#667)) ([e8ed9f2](e8ed9f2))
* improve pytest-xdist compatibility ([9b9090f](9b9090f))
* lru_cache on snapshot reads ([#629](#629)) ([c1a675f](c1a675f))
* remove legacy path usage to support no:legacypath, closes [#677](#677) ([#684](#684)) ([6385979](6385979))

### Code Refactoring

* simplify data serializer for ambr ([#676](#676)) ([3d296e1](3d296e1))
* write performance improvements, api clarity ([#645](#645)) ([2c31c39](2c31c39))

### Features

* **json:** serialize None as null, close [#622](#622) ([c330680](c330680))
* numerically sort snapshots if possible, close [#657](#657) ([4ca0716](4ca0716))
* **serializer:** preserve key ordering of OrderedDict ([0a2289a](0a2289a))
* support overriding the amber serializer class ([#683](#683)) ([662c93f](662c93f))
* update python version, pytest version ([#658](#658)) ([c360b95](c360b95))

### BREAKING CHANGES

* Serializers may now throw a TaintedSnapshotError which will tell the user to regenerate the snapshot even if the underlying data has not changed. This is to support rolling out more subtle changes to the serializers, such as the introduction of serializer metadata.
* Renamed DataSerializer to AmberDataSerializer.
* **serializer:** Key order is now preserved if using OrderedDict in both the Amber serializer and JSON serializer.
* **json:** The JSONSnapshotExtension now serializes Python's None as "null" rather than "None".
* Raise minimum python version to 3.8.1 and min. pytest version to v7.
* PyTestLocation.filename has been renamed to .basename

* refactor: add test_location kwarg to get_snapshot_name

* refactor: get_snapshot_name is now static as a classmethod

* refactor: remove pre and post read/write hooks
* Pre and post read/write hooks have been removed without replacement to make internal refactor simpler. Please open a GitHub issue if you have a use case for these hooks.

* refactor: rename Fossil to Collection
* The term 'fossil' has been replaced by the clearer term 'collection'.

* refactor: pass test_location to read_snapshot

* refactor: remove singular write_snapshot method

* refactor: dirname property to method

* refactor: pass test_location to discover_snapshots

* refactor: remove usage of self.test_location

* refactor: make write_snapshot a classmethod

* refactor: do not instantiate extension with test_location
* Numerous instance methods have been refactored as classmethods.
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.

1 participant