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

Where relevant, replace equality checks in assert! with assert_eq! #1124

Merged
merged 1 commit into from
Sep 21, 2019

Conversation

AdminXVII
Copy link
Contributor

assert_eq! gives more debug info when the test fails by default than assert!. This should help make debugging easier.

@asomers
Copy link
Member

asomers commented Sep 16, 2019

Thanks! This looks great. BTW, I hope to have the test failures fixed soon in PR #1123 .

@AdminXVII
Copy link
Contributor Author

Cool!

@asomers
Copy link
Member

asomers commented Sep 17, 2019

I don't understand. What problem did that last commit fix?

@AdminXVII
Copy link
Contributor Author

On mac, the kevent struct use a packed alignment (according to the previously failing tests). assert_eq took a reference to the field, which is unsafe because the value can be unaligned. By adding brackets, the value is copied, so it's correctly aligned.

@asomers
Copy link
Member

asomers commented Sep 17, 2019

I don't think that's true, at least not in general. Braces can certainly capture references. For this specific case, how about removing the assignment of expected altogether? It's not necessary. Instead, you can put literals directly into the assert_eq! statements.

@AdminXVII
Copy link
Contributor Author

Braces can capture references because they are copy. Since what's inside the brace is a copy value, not a reference, this is not a problem, even if the macro expands to something that takes the reference of it. That said I fixed it with your suggestion.

@asomers
Copy link
Member

asomers commented Sep 20, 2019

Please squash the commits and I'll merge. Oh, and you'll probably have to rebase now that PR #1123 is done.

`assert_eq!` gives more debug info when the test fails by default than
`assert!`. This should help make debugging easier.
@AdminXVII
Copy link
Contributor Author

@asomers Done

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Sep 21, 2019
1124: Where relevant, replace equality checks in assert! with assert_eq! r=asomers a=AdminXVII

`assert_eq!` gives more debug info when the test fails by default than `assert!`. This should help make debugging easier.

Co-authored-by: Xavier L'Heureux <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 21, 2019

Build succeeded

@bors bors bot merged commit bbc42e7 into nix-rust:master Sep 21, 2019
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