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

Stricter panic assertions #69

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

tommy-gilligan
Copy link
Contributor

Assertions on the message of the panic could be helpful for further development eg. adding labels to expectations (they would be included in messages)
Also helps tests document themselves better without comments.

@tommy-gilligan tommy-gilligan marked this pull request as ready for review May 16, 2023 07:23
@dbrgn
Copy link
Owner

dbrgn commented Jul 30, 2023

Thanks, this looks like a good idea!

Could you rebase your branch against current main?

@tommy-gilligan
Copy link
Contributor Author

I ended up redoing the work and force pushing. Felt I might as well because the work hadn't yet been done for eh1 anyways. Hope the force push isn't too confusing.

On the earlier version of this work I was matching on substrings

#[should_panic(expected = "i2c::write data does not match expectation")]

In this redo, I have gone with matching on the full string

#[should_panic(expected = "assertion failed: `(left == right)`\n  left: `[1, 2]`,\n right: `[1, 3]`: i2c::write data does not match expectation")]

This feels like maybe it's a bit extra. Please let me know your preference.

@dbrgn
Copy link
Owner

dbrgn commented Aug 4, 2023

This feels like maybe it's a bit extra. Please let me know your preference.

I'm a bit unsure. I probably would have included only substrings, but the full messages also have some advantages, so I'll merge this as-is for now.

@dbrgn dbrgn merged commit 04f0ef5 into dbrgn:main Aug 4, 2023
3 checks passed
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