mw/com: Add test for interface containg events, fields and methods - #1018
mw/com: Add test for interface containg events, fields and methods#1018bemerybmw wants to merge 2 commits into
Conversation
0978078 to
ea980ae
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
5e8f9b6 to
cb67d56
Compare
cb67d56 to
6beb1db
Compare
| * terms of the Apache License 3.0 which is available at | ||
| * https://www.apache.org/licenses/LICENSE-1.0 | ||
| * | ||
| * SPDX-License-Identifier: Apache-1.0 |
| namespace score::mw::com::test | ||
| { | ||
|
|
||
| std::string ParseServiceInstanceManifest(int argc, const char** argv) |
There was a problem hiding this comment.
I do not think we should have this tiny function in the global common resources. There is already too much stuff in there and no one has the overview, which leads to less usage of the common resources. ParseCommandLineArguments already exists as a general purpose function and this minor specialization really does not deserve to be part of the common resources for all tests.
| "appDesc": "all_service_elements", | ||
| "logLevel": "kDebug", | ||
| "logLevelThresholdConsole": "kDebug", | ||
| "logMode": "kRemote|kConsole" |
There was a problem hiding this comment.
| "logMode": "kRemote|kConsole" | |
| "logMode": "kRemote" |
There was a problem hiding this comment.
This will stop double printing of the loggs
|
|
||
| typename Trait::template Method<void(TestType, TestType)> with_in_args_only{*this, "with_in_args_only"}; | ||
|
|
||
| typename Trait::template Field<TestType, WithGetter> getter_only_enabled_field{*this, "get_only_enabled_field"}; |
There was a problem hiding this comment.
if there is no good reason for it I would change getter to get, to have same naming in the config and code
| { | ||
|
|
||
| void RunConsumer(); | ||
| void run_consumer(const score::cpp::stop_token& stop_token); |
There was a problem hiding this comment.
This snake case naming inconsistency started with me doing it wrong once, and now it's spreading 😆
I do not have strong opinions about it but RunConsumer would be compliant to our guidelines
|
|
||
|
|
||
| def test_basic_acceptance_same_process_test(target): | ||
| """Test LoLa functionality between provider and consumer which contain field, methods and events in the same process. |
There was a problem hiding this comment.
| """Test LoLa functionality between provider and consumer which contain field, methods and events in the same process. | |
| """Test LoLa functionality between provider and consumer which contain fields, methods and events in the same process. |
| features = COMPILER_WARNING_FEATURES + [ | ||
| "aborts_upon_exception", | ||
| ], | ||
| visibility = ["//score/mw/com/test/fields:__pkg__"], |
There was a problem hiding this comment.
| visibility = ["//score/mw/com/test/fields:__pkg__"], | |
| visibility = ["//score/mw/com/test/all_service_elements:__pkg__"], |
There was a problem hiding this comment.
I do not think cc_binaries need visibility at all though. Since they are packaged right away in this BUILD file
| proxy.with_in_args_and_return(kInArgsAndReturnMethodTestValueA, kInArgsAndReturnMethodTestValueB); | ||
| if (!with_in_args_and_return_result.has_value()) | ||
| { | ||
| FailTest("Consumer: with_in_args_and_return call failed: ", with_in_args_and_return_result.error()); |
There was a problem hiding this comment.
Should all fail tests use kFailureMessagePrefix?
| std::cout << "\nConsumer: Step 3 - Test events" << std::endl; | ||
| SubscribeAndReceiveEventsOrFail(proxy.event_1, kEvent1ValuesToSend, stop_token); | ||
| SubscribeAndReceiveEventsOrFail(proxy.event_2, kEvent2ValuesToSend, stop_token); | ||
| SubscribeAndReceiveEventsOrFail(proxy.get_and_notifier_enabled_field, kAllGetAndNotifierValues, stop_token); |
There was a problem hiding this comment.
You are subscribing and blocking on each Event sequentially same happens later with methods and fields. So you are synchronising every service element interaction by hand. Which negates some advantages of having multiple service elements in the same test. This way we will never detect if LoLa has any problems with handling multiple service elements.
| std::cout << "\nProvider: Step 4 - Set initial field values" << std::endl; | ||
| UpdateField(service.getter_only_enabled_field, kGetterOnlyFieldInitialValue); | ||
| UpdateField(service.set_and_get_enabled_field, kSetAndGetFieldInitialValue); | ||
| UpdateField(service.get_and_notifier_enabled_field, kGetAndNotifierFieldInitialValue); |
There was a problem hiding this comment.
I see the same artificial synchronization issue here as with the receiver.
I think the test would be stronger if you would issue the Send/Update calls first and wait on all of them at once later. That way LoLa will have to handle the correct synchronization
3 participants
No description provided.