-
Notifications
You must be signed in to change notification settings - Fork 33
Adding checks for device support to assumption tests #32
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
Conversation
|
Weird, i thought I already had all those in place, not sure where they went. As for the [WARN] I vaguely remember that was there for a reason. |
|
LGTM (one small change requested) |
TESTS/assumptions/SPI/SPI.cpp
Outdated
| @@ -1,3 +1,8 @@ | |||
| // check if SPI is supported on this device | |||
| #if !DEVICE_SPI | |||
| #error SPI is not supported on this platform, add 'DEVICE_SPI' definition to your platform. | |||
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.
why [NOT_SUPPORTED] not in this one?
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.
green tea magic as i recall. Cant remember why though. Lets remove it and see what breaks eh?
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.
Oh nope, I was just copy pasting from the TESTS/api/SPI test and apparently that one gets it wrong as well! I will send a patch! Thanks for the catch!
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.
did you make sure to test your changes on multiple boards to make sure it doesnt break SPI?
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 double checked compilation on K64F and DELTA_DFCM_NNN50 and it looked good to me.
Perhaps we should add some Travis CI checks to double check compilation.... 😜
|
I've updated the PR with your requested changes @0xc0170, would you mind taking a look? |
This adds the compile-time checks that allows builds to show up as "SKIPPED". For targets that don't support the given APIs, I've added the skip check so it's more apparent what is and what isn't being tested.