MarvinJWendt / testza

Full-featured test framework for Go! Assertions, fuzzing, input testing, output capturing, and much more! 🍕
MIT License
418 stars 21 forks source link

Remove capacity from snapshots #185

Closed jamestelfer closed 1 year ago

jamestelfer commented 1 year ago

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 cap can introduce, my preference would be to go ahead outside of a v2 style 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 ofof go-spew's ConfigState struct 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.

codecov[bot] commented 1 year ago

Codecov Report

Base: 87.02% // Head: 87.02% // No change to project coverage :thumbsup:

Coverage data is based on head (2eccb5c) compared to base (74d1242). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #185 +/- ## ======================================= Coverage 87.02% 87.02% ======================================= Files 10 10 Lines 740 740 ======================================= Hits 644 644 Misses 58 58 Partials 38 38 ``` | [Impacted Files](https://codecov.io/gh/MarvinJWendt/testza/pull/185?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marvin+Wendt) | Coverage Δ | | |---|---|---| | [snapshot.go](https://codecov.io/gh/MarvinJWendt/testza/pull/185/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marvin+Wendt#diff-c25hcHNob3QuZ28=) | `83.78% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marvin+Wendt). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marvin+Wendt)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jamestelfer commented 1 year ago

Note: the test failure appears to be transient and unrelated, but I can't re-run

MarvinJWendt commented 1 year ago

Hi, thanks for the well thought through and clean PR! I agree that it makes sense to not save the capacity by default.

Given the test instability that cap can introduce, my preference would be to go ahead outside of a v2 style version upgrade, but I'd like to get your feedback.

Testza is still at v0.x.x, which means that we can introduce breaking changes without needing to bump the major version, and I also agree that this change is only partially breaking, as the API remains untouched, but I'll go with a v0.+1.x bump, as I think we should indicate that something might not work as expected (https://semver.org/#spec-item-4)

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.

Good point. I think we should consider adding an option to overwrite the spew config with a custom one. Something like testza.SetSpewConfig. Then users could manually tweak their snapshot files. Arguably, we should also consider that some people might want to use different configs for different snapshots, but I think this is out of scope for this PR.

Note: the test failure appears to be transient and unrelated, but I can't re-run

Yes, that specific test is acting weird lately. I'll fix it in another PR :)

MarvinJWendt commented 1 year ago

Released in v0.5.0 :)