-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Tests for 22 #42
Tests for 22 #42
Conversation
90051c3
to
49c1855
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing some issues in the implementation and I'm not sure the new test is indeed testing what it's meant to do so I'd like my inline comments to be addressed before a take a closer look at the actual assertions.
@japaric I've addressed your comments, so I think it is good for a review again 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than my request to use a Cargo feature named "lm3s6965" instead of "default", this looks good to me! feel free to send it to bors with or without my request fulfilled
bors r+ |
Build succeeded: |
Also partially depends on #40 and fixes #26, by implementing tests for #22.