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

Implement current integrations tests as rust tests #54

Closed

Conversation

AnthonyMichaelTDM
Copy link
Contributor

@AnthonyMichaelTDM AnthonyMichaelTDM commented May 8, 2024

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).

uses the goldenfile crate to fail tests if the "goldenfile" is changed

merging will require updating both Wulf#53 (to add the tests to the CI) and  Wulf#51 (to add a test for const numeric enums), or if those are merged first then this will need to be updated.
@AnthonyMichaelTDM
Copy link
Contributor Author

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.

@Wulf
Copy link
Owner

Wulf commented May 12, 2024

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?

@AnthonyMichaelTDM
Copy link
Contributor Author

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.

@AnthonyMichaelTDM
Copy link
Contributor Author

AnthonyMichaelTDM commented May 12, 2024

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

@AnthonyMichaelTDM
Copy link
Contributor Author

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)

@Wulf
Copy link
Owner

Wulf commented May 19, 2024

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?

@AnthonyMichaelTDM
Copy link
Contributor Author

AnthonyMichaelTDM commented May 19, 2024 via email

@AnthonyMichaelTDM
Copy link
Contributor Author

just to confirm, we're leaving this out right? if so I"m going to close the PR

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.

2 participants