Remove capacity from snapshots #185
Conversation
Include pointers and slices in a snapshot object, shows configuration of go-spew, more indicative of real world usage.
Separate concern into shared file so go-spew configuration is only stated once.
Instead of modifying the global state and restoring it, use the concurrent configuration object to configure go-spew. This removes the need for defer, and makes tests with snapshot marginally safer if tests are run concurrently. Mostly though, it's a little cleaner, and makes it more straightforward to add more configuration.
Capacities may vary across runtimes, creating instability in saved snapshots.
Codecov ReportBase: 87.02% // Head: 87.02% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #185 +/- ##
=======================================
Coverage 87.02% 87.02%
=======================================
Files 10 10
Lines 740 740
=======================================
Hits 644 644
Misses 58 58
Partials 38 38
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Note: the test failure appears to be transient and unrelated, but I can't re-run |
|
Hi, thanks for the well thought through and clean PR! I agree that it makes sense to not save the capacity by default.
Testza is still at
Good point. I think we should consider adding an option to overwrite the spew config with a custom one. Something like
Yes, that specific test is acting weird lately. I'll fix it in another PR :) |
|
Released in |
Changes the snapshot functionality such that capacities for slices are no longer emitted into the created snapshot files. Slice capacities can vary across different runtimes, creating the potential for intermittent snapshot comparison failures.
While this doesn't break the API surface, it does change behaviour is such a way that will cause existing tests that save capacities to fail when they update the library version. Given the test instability that
capcan introduce, my preference would be to go ahead outside of av2style version upgrade, but I'd like to get your feedback.Another point of possible contention is that Arrays also have a capacity which is not variable, and there is a possibility that varying such a capacity would be a semantic error that a user of the library would want to catch.
Fixes #184
Notes
In order to make this change safely, I've introduced an additional test to expose the behaviour, and pulled the
Sdump()calls into a separate function. This allows for the use ofofgo-spew'sConfigStatestruct instead of the global config. This makes varying configuration more straightforward, as it doesn't need to be restored, and removes the possibility of strange errors caused by concurrency in tests.