mw/com : Add the integration tests for moving skeletonMethod. - #659
mw/com : Add the integration tests for moving skeletonMethod.#659Tejveerpratap2803 wants to merge 3 commits into
Conversation
612b2cc to
a5181ea
Compare
Tejveerpratap2803
changed the title
27dc852 to
d59adc0
Compare
|
|
||
| void RunConsumer(const SkeletonMoveScenario& scenario, const score::cpp::stop_token& stop_token) | ||
| { | ||
| auto proxy_done_sync = ProcessSynchronizer::Create(kProxyDoneShmPath); |
There was a problem hiding this comment.
We generally avoid abbreviations: proxy_done_process_synchronizer
There was a problem hiding this comment.
Removed now we are using concurrency::Notification.
| "instances": [ | ||
| { | ||
| "instanceId": 1, | ||
| "asil-level": "QM", |
There was a problem hiding this comment.
I think in general, we should use asil b by default unless there's a good reason not to. The most important thing is that our code works for asil b applications. Also, an asil b provider sets up infrastructure for both asil b and qm, so we're technically testing more code paths.
|
|
||
| enum class SkeletonMoveScenario : std::uint8_t | ||
| { | ||
| kMoveConstructBeforeOffered = 0, |
There was a problem hiding this comment.
No need to manually specify the numbers unless they're actually important e.g. if the counting starts from a non zero value.
|
|
||
| // Step 3. Move construct: Skeleton B = std::move(Skeleton A) [before OfferService] | ||
| std::cout << "\nProvider: Step 3 - Move construct Skeleton B = std::move(Skeleton A)" << std::endl; | ||
| auto skeleton_b = std::move(skeleton_a); |
There was a problem hiding this comment.
What about moving once before RegisterHandlerOrFail and again here?
There was a problem hiding this comment.
Refined the code
| } | ||
|
|
||
| // Step 3. For after-offered scenarios: signal provider that proxy/proxies are created | ||
| if (IsAfterOffered(scenario)) |
There was a problem hiding this comment.
Having a single function and using these flags makes the test harder to read IMO. I think it's clearer to have separate functions for the different tests to make the sequences clear.
| { | ||
| std::random_device rd{}; | ||
| std::mt19937 gen{rd()}; | ||
| std::uniform_int_distribution<int> dist{0, 500}; |
There was a problem hiding this comment.
We shouldn't have a magic number here of 500. This value is very important for the test so should be a constant (probably defined in test_parameters).
| if (IsAfterOffered(scenario)) | ||
| { | ||
| std::cout << "\nConsumer: Step 3 - Notify provider: proxy/proxies created" << std::endl; | ||
| proxy_done_sync->Notify(); |
There was a problem hiding this comment.
After discussing with @limdor, we agreed that we shouldn't go with the approach of calling move in a loop which is more of a stress test. This minimises load in the ci and more importantly keeps the test sequence clearer i.e. that it is just testing one thing, i.e. what happens when we move the handler once. Since we have the nightly job which runs the test many times, we should get a lot of runs with different timings. Based on this and our discussion on Friday, I think that we should have 4 tests:
- move construct before offer
- move construct after offer (fuzzy)
- move assign before offer
- move assign after offer (fuzzy)
Before offer
The before offer tests should move the skeleton twice: before and after registering the handler (it then offers the service). After it detects that the first method handler was called (using a concurrency::Notification: it should notify a concurrency::Notification to indicate that the handler was called. the main thread of the test should wait on this notification), it should then register a new handler an once that handler is called, it can finish the test which destroys the skeleton / stop offers the service.
In the consumer, it should calling the first method. It should then keep calling the method until it gets a return value uniquely identifying the second handler and then can finish. If I haven't missed something, I think this sequence would not even require a ProcessSynchronizer for indicating to the skeleton when the consumer is done (since it gets this when the last registered handler is called).
After offer
The provider should offer the service and then sleep a random time and then move the skeleton. After this, it should register a new handler and repeat (similar to the before offer test). The consumer should behave the same as in the before offer test.
The "fuzzy" behaviour is basically part of the after offer tests.
2f4d6d4 to
22287bc
Compare
22287bc to
6c66546
Compare
3 participants
Adding integration tests that verify a Skeleton (and its SkeletonMethod) can be move-constructed or move-assigned without disrupting method handler registration or invocation.
Issue : #483