Conversation
acmorrow
commented
Feb 4, 2025
acmorrow
commented
- Run all tests in the workspace and do not fail fast
- Avoid need to reiterate PGN numbers in the PGN enum macro.
|
|
||
| macro_rules! define_pgns { | ||
| ( $(($pgndef:ident, $pgn:expr)),* ) => { | ||
| ( $($pgndef:ident),* ) => { |
There was a problem hiding this comment.
I didn't push more for this change in #408 because I wasn't completely sure there wasn't a reason it didn't work, but it sure seems to.
There was a problem hiding this comment.
huh, ok. Sounds good to me
There was a problem hiding this comment.
I guess the one thing to check is whether this NmeaMessageBody type actually has any tests? If you want, I'll add one.
|
|
||
| test: | ||
| cargo test -p micro-rdk --lib --features native,ota | ||
| cargo test --workspace --tests --no-fail-fast --features native,ota |
There was a problem hiding this comment.
what is --no-fail-fast?
There was a problem hiding this comment.
It prevents a failing test from stopping the execution of other tests. I'd much prefer to learn in one go all the tests I broke, rather than it stopping on the first failure.
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
Updated, PTAL. I also fixed some new warnings that popped up with the expanded testing.
| let nmea_message = NmeaMessage::try_from(data); | ||
| assert!(nmea_message.is_ok()); | ||
|
|
||
| let nmea_message = nmea_message.unwrap(); | ||
| let message = match nmea_message.data { | ||
| NmeaMessageBody::GnssSatsInView(val) => Some(val), | ||
| _ => None, | ||
| }; | ||
| assert!(message.is_some()); | ||
| let message = message.unwrap(); | ||
|
|
There was a problem hiding this comment.
@gvaradarajan - New since last review, this now ensures that the NmeaMessage enum gets tested at least once.
There was a problem hiding this comment.
thank you for adding this
| pub struct NmeaMessage { | ||
| metadata: NmeaMessageMetadata, | ||
| data: NmeaMessageBody, | ||
| pub metadata: NmeaMessageMetadata, |
There was a problem hiding this comment.
Needed for testing, let me know if you would prefer accessors, but I find them less interesting in a language where immutability is the default.
There was a problem hiding this comment.
if it's just for testing, would pub(crate) work?
There was a problem hiding this comment.
Ah, yes, I think it would. I always forget that that is an option. Too accustomed to C++ having only public/private. If it works I'll make that change before push.
| let nmea_message = NmeaMessage::try_from(data); | ||
| assert!(nmea_message.is_ok()); | ||
|
|
||
| let nmea_message = nmea_message.unwrap(); | ||
| let message = match nmea_message.data { | ||
| NmeaMessageBody::GnssSatsInView(val) => Some(val), | ||
| _ => None, | ||
| }; | ||
| assert!(message.is_some()); | ||
| let message = message.unwrap(); | ||
|
|
There was a problem hiding this comment.
thank you for adding this
| pub struct NmeaMessage { | ||
| metadata: NmeaMessageMetadata, | ||
| data: NmeaMessageBody, | ||
| pub metadata: NmeaMessageMetadata, |
There was a problem hiding this comment.
if it's just for testing, would pub(crate) work?
| } | ||
|
|
||
| pub fn get_periodic_app_client_tasks(&mut self) -> Vec<Box<dyn PeriodicAppClientTask>> { | ||
| #[allow(unused_mut)] |
|
@gvaradarajan - Oops. I thought I'd committed and pushed the change to |
2 participants
