mw/com: Add the integration tests for moving proxyEvent#607
mw/com: Add the integration tests for moving proxyEvent#607sahithi-nukala wants to merge 3 commits into
Conversation
sahithi-nukala
force-pushed
the
sah_proxy_event_move_tests
branch
from
544058d to
49a2440
Compare
sahithi-nukala
force-pushed
the
sah_proxy_event_move_tests
branch
6 times, most recently
from
94c8dd5 to
f486d86
Compare
sahithi-nukala
force-pushed
the
sah_proxy_event_move_tests
branch
from
f486d86 to
c755a1c
Compare
sahithi-nukala
requested review from
LittleHuba,
castler,
crimson11,
hoe-jo and
limdor
as code owners
| const std::size_t num_send_iterations, | ||
| const score::cpp::stop_token& stop_token) | ||
| { | ||
| const auto moved_to_name = filesystem::Path{kInstanceSpecifierMovedTo.ToString()}.Filename().Native(); |
There was a problem hiding this comment.
Do we need to add moved_to_name to kInterprocessNotificationShmPath?
| const auto initial_value = static_cast<std::uint32_t>(iteration * num_samples_to_send) + 1U; | ||
| std::cout << "\nProvider: Iteration " << (iteration + 1U) << " of " << num_send_iterations << " - Send " | ||
| << num_samples_to_send << " samples" << std::endl; | ||
| SendSamples(skeleton_container.GetSkeleton(), num_samples_to_send, initial_value); |
There was a problem hiding this comment.
If you want to extract this into a function, then the name should be more descriptive IMO. Maybe something like SendIncrementingSequenceOfSamples. Otherwise passing in "initial_value" is a bit strange to me
|
|
||
| // Step 2. Move construct proxy before subscribe | ||
| std::cout << "\nConsumer: Step 2 - Move construct proxy before subscribe" << std::endl; | ||
| auto moved_proxy = proxy_container.Extract(); |
There was a problem hiding this comment.
IMO, it's better to make the move construction explicit like in the skeleton move test:
skeleton_container.CreateSkeleton(kInstanceSpecifierMovedTo, "skeleton_event_move_semantics");
auto original_skeleton = skeleton_container.Extract();
// Step 2. Move construct skeleton
std::cout << "\nProvider: Step 2 - Move construct skeleton" << std::endl;
auto moved_to_skeleton = std::move(original_skeleton);
| namespace score::mw::com::test | ||
| { | ||
|
|
||
| inline auto MakeSampleSequenceCallback(std::optional<std::uint32_t>& latest_value, const char* failure_message_prefix) |
There was a problem hiding this comment.
IMO, these functions are quite specific to the move semantic tests, so I wouldn't put them in the test resources.
| for (std::size_t iteration = 0U; iteration < num_iterations; ++iteration) | ||
| { | ||
| std::cout << "\nConsumer: Iteration " << (iteration + 1U) << " of " << num_iterations << std::endl; | ||
| const bool subscribed = |
There was a problem hiding this comment.
Why are you waiting for a state change on every iteration?
|
|
||
| // Step 6. Wait for provider to send the first batch of values and notify | ||
| std::cout << "\nConsumer: Step 6 - Receive first batch of samples" << std::endl; | ||
| ReceiveAndNotify(proxy_event_receiver, |
There was a problem hiding this comment.
In general, I don't think it's a good idea to extract too much actual test logic. We want the test sequence to be as clear as possible. Something like proxy_state_change_notifier.WaitForStateChange or proxy_event_receiver.WaitForSamples are basically doing one thing and it's completely clear from the name what that is. Here, looking at the name and the args, it's not really clear what it does.
e.g. the name doesn't hint that it's also checking the subscription state (which I'm not sure it should be there at all), it's not really clear that it will receive samples, notify and then reset the process_synchronizer and then repeat that multiple times. IMO, that logic should be here in the main test sequence or it for example you could make it simpler like ReceiveSamplesAndNotify but do the loop here and reset the process_synchronizer here. But tbh at that point, I don't see much point in the function. I also think the fact that the function takes 6 params and has 3 template params is a bit of a code smell.
There was a problem hiding this comment.
This is also not an exact science but is a bit of developer preference. We can also get an outside opinion. @limdor what do you think?
|
|
||
| // Step 7. Unsubscribe and subscribe again across the provider's re-offer | ||
| std::cout << "\nConsumer: Step 7 - Unsubscribe and subscribe again" << std::endl; | ||
| ResubscribeAcrossReoffer(moved_proxy.moved_event_, |
There was a problem hiding this comment.
similar to my comments above. This should either be made clearer or put here in the main.
| { | ||
| std::cout << "\nConsumer: Waiting for provider to withdraw its offer" << std::endl; | ||
| const bool withdrawn = | ||
| proxy_state_change_notifier.WaitForStateChange(stop_token, SubscriptionState::kSubscriptionPending); |
There was a problem hiding this comment.
kSubscriptionPending shouldn't really be used to determine whether a provider is alive or not. I think you should probably use a ProcessSynchronizer instead. Although to be honest, I think it would also be fine to unsubscribe and subscribe without the provider reoffering. Is there a particular reason why you want to wait for the skeleton to reoffer?
| ProxyEventReceiver proxy_event_receiver{ | ||
| original_proxy.moved_event_, | ||
| MakeSampleSequenceCallback(latest_value, "proxy_event_move_semantics consumer failed:")}; | ||
| ProxyEventStateChangeNotifier proxy_state_change_notifier{original_proxy.moved_event_}; |
There was a problem hiding this comment.
We want to make sure that the set receive handler is not affected when moving. so this should not be destroyed and we shouldn't create a new one.
| auto process_synchronizer_result = | ||
| ProcessSynchronizer::CreateUniquePtr(kInterprocessNotificationShmPath + std::string{moved_to_name}); | ||
|
|
||
| // Step 1. Create and offer the skeleton the consumer receives samples from. A second instance is also offered so |
There was a problem hiding this comment.
You could have both proxy instances connect to the same skeleton?
2 participants
Adding the integration tests that verify a Proxy (and its ProxyEvent) can be moved without disrupting service discovery, subscription, or sample reception.
Issue: #483