Open AnthonyMichaelTDM opened 5 months ago
The way I see it, the current tests should still remain as tests for the cli, which is why they weren't removed or modified by this PR.
Anothony, thanks for introducing me to goldenfile
. Not sure I want to buy into it though. What benefits do you see in using it apart from a seamless integration into cargo test
and all the benefits that brings?
Honestly, that's about it. The integration with cargo test is the main selling point, having all our tests (current and future) tied to the same tool is quite nice and if something breaks we'll be able to see what example is breaking whereas with the current setup we'd just see that the tests failed. I understand if that's an unconvincing argument and you're free to close the PR if that's the case, I just had some extra time and thought I'd throw together a PR.
Also, if you would like to merge this, give me time to fix the failing CI, cargo check is failing and I need to investigate that
update: Okay, I just need to update the tests to account for the changes from #51
another pro is being able to distinguish between whether an issue exists in the CLI or the library itself (if existing tests fail but new ones don't, the CLI is the issue whereas if both fail then it's probably a library issue)
Thanks for sharing those details @AnthonyMichaelTDM. For the sake of simplicity, I prefer we leave this one out. Testing the CLI/lib in two ways sounds more complicated than needs be.
As for the niceties of integrating with cargo test
, I truly do get it.
That being said, would you care to join me in maintaining this repo? what about dsync
?
Sure, I’m down to help maintain these repos
uses the goldenfile crate to fail tests if the "goldenfile" is changed
merging will require updating both #53 (to add the tests to the CI) and #51 (to add a test for const numeric enums), or if those are merged first then this will need to be updated (which is probably the better approach).