Add unit test in skeleton_event.h for checking timestamp #87 #125
Conversation
ShoroukRamzy
requested review from
LittleHuba,
castler and
hoe-jo
as code owners
ShoroukRamzy
changed the title
|
Hi @LittleHuba , @castler and @hoe-jo, Could you please let me know your feedbacks and help me merge this PR if possible? I don't have sufficient permissions to merge. Thanks! |
ShoroukRamzy
force-pushed
the
skeleton-event-ts-test
branch
from
3b60016 to
11eccb3
Compare
LittleHuba
left a comment
LittleHuba
left a comment
There was a problem hiding this comment.
Apologies for the delay, we had vacation over Christmas and New Year.
I left you some comments. In general the test is really good. I would only prefer if we can make it more resilient against unrelated changes.
As a bonus you could try to make more variables const.
| ASSERT_TRUE(first_send_result.has_value()); | ||
|
|
||
| // THEN its timestamp should be a valid, non-zero value | ||
| auto first_final_slot_status = EventSlotStatus(first_slot_indicator.GetSlotQM().load()); |
There was a problem hiding this comment.
| auto first_final_slot_status = EventSlotStatus(first_slot_indicator.GetSlotQM().load()); | |
| const EventSlotStatus first_final_slot_status{first_slot_indicator.GetSlotQM().load()}; |
|
|
||
| // THEN its timestamp should be a valid, non-zero value | ||
| auto first_final_slot_status = EventSlotStatus(first_slot_indicator.GetSlotQM().load()); | ||
| EXPECT_FALSE(first_final_slot_status.IsInWriting()); |
There was a problem hiding this comment.
This assert is IMHO not relevant for this test. Better remove it to make this test less impacted by unrelated changes in the codebase.
| auto second_initial_status = EventSlotStatus(second_slot_indicator.GetSlotQM().load()); | ||
| ASSERT_TRUE(second_initial_status.IsInWriting()); |
There was a problem hiding this comment.
| auto second_initial_status = EventSlotStatus(second_slot_indicator.GetSlotQM().load()); | |
| ASSERT_TRUE(second_initial_status.IsInWriting()); |
| auto first_initial_status = EventSlotStatus(first_slot_indicator.GetSlotQM().load()); | ||
| ASSERT_TRUE(first_initial_status.IsInWriting()); |
There was a problem hiding this comment.
This assert is IMHO not relevant for this test. Better remove it to make this test less impacted by unrelated changes in the codebase.
| auto first_initial_status = EventSlotStatus(first_slot_indicator.GetSlotQM().load()); | |
| ASSERT_TRUE(first_initial_status.IsInWriting()); |
| auto second_final_slot_status = EventSlotStatus(second_slot_indicator.GetSlotQM().load()); | ||
| EXPECT_FALSE(second_final_slot_status.IsInWriting()); |
There was a problem hiding this comment.
| auto second_final_slot_status = EventSlotStatus(second_slot_indicator.GetSlotQM().load()); | |
| EXPECT_FALSE(second_final_slot_status.IsInWriting()); | |
| const EventSlotStatus second_final_slot_status{second_slot_indicator.GetSlotQM().load()}; |
|
I wonder if the test case touches any requirement and therefore should make use of test properties. |
3 participants
Hi @LittleHuba,
This PR resolves issue #87 and verifies that SkeletonEvent::Send correctly updates the timestamp in the shared memory control data.
Thanks!