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

Add with-assert-override #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luke-biel
Copy link
Collaborator

@luke-biel luke-biel commented Dec 14, 2022

Closes #109
Closes #84

Copy link

@vriesk vriesk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

This should work, though the need to invoke additional macro is somewhat clunky.

Also, in case of pretty_assertions, it does not have its own assert! macro, only assert_eq! and assert_ne!. I'm unsure what's the use case for overriding of assert! is.

@luke-biel
Copy link
Collaborator Author

Yeah, it's clunky, however there's no better way without providing custom test harness.
So far we can consider crate level, module level and test level configurations. Crate level require something that can be guaranteed in static context, and this is best solution I could think of. Module level would require to have written custom macro with module harness, and this is way to big of a task to do it now. For test level I've already shown few workarounds with using, with or simply doing assertion comparison by yourself.

As for assert being present - as I mentioned, we need something generic over different possible assertion crates. It's optional to override assert, as shown in tests.

@vriesk
Copy link

vriesk commented Jan 2, 2023

crate level would certainly be most user-friendly, with a possible module-level override option (not my use case, but I imagine this being useful).

@luke-biel
Copy link
Collaborator Author

I'm gonna fix tests in this and push it then.

I'd really avoid feature flags, as they tend to break when workspaces are at play, plus it would anyway introduce dependency on pretty-assertions, one that shouldn't break easily, but I had enough bad experience with insta and broken transitive deps to avoid depending on other crates at all cost.
I don't really see a better option without using nightly or forcing a custom test harness. Maybe in the future I can replace this once something new shows up in rust. For time being unfortunately it has to be lib.rs level macro.

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.

Does not fully work with pretty_assertions, fix OK? Allow replacement of test-case assertions on crate scale
2 participants