Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/Remote Control - Allow Multiple Modules per Module Type #2226

Merged
merged 53 commits into from
Sep 4, 2019
Merged

Feature/Remote Control - Allow Multiple Modules per Module Type #2226

merged 53 commits into from
Sep 4, 2019

Conversation

LitvinenkoIra
Copy link
Contributor

ATF Test Scripts to check #2919

This PR is [ready] for review.

Summary

Scripts for test feature "Remote Control - Allow Multiple Modules per Module Type"

ATF version

develop

CLA

@AStasiuk AStasiuk requested a review from mrapitis August 6, 2019 06:52
@dboltovskyi dboltovskyi force-pushed the feature/multiple_modules branch from a77e9ca to 44a50fa Compare August 7, 2019 14:30
@LitvinenkoIra LitvinenkoIra mentioned this pull request Aug 15, 2019
Merged
1 task
aderiabin added 21 commits August 19, 2019 12:15
@aderiabin
Update of core modules of user_modules
4b3ceba
@aderiabin
Update of hmi_values core module to support multiple modules for modu…
beaf7b1 
…le type
@aderiabin
Add remote_control module as new core module of user_modules
859b7d6
@aderiabin
Add common module for multiple modules feature
c57056f 
FixUp
@aderiabin
Add RC capability related scripts for multiple modules feature
6a043d6
@aderiabin
Add SeatLocation capability related scripts for multiple modules feature
f860bf1
@aderiabin
Add GetInteriorVehicleData RPC related scripts for multiple modules f…
fcb4c9e 
…eature
@aderiabin
Add SetInteriorVehicleData RPC related scripts for multiple modules f…
93ebbcb 
…eature
@aderiabin
Add OnInteriorVehicleData notification related scripts for multiple m…
85bd8f6 
…odules feature
@aderiabin
Add ButtonPress RPC related scripts for multiple modules feature
ab8519a
@aderiabin
Add RC modules allocation related scripts (no multipleAccess) for mul…
b862e3c 
…tiple modules feature
@aderiabin
Add RC modules allocation related scripts (multipleAccess) for multip…
3f234cf 
…le modules feature
@aderiabin
Add RC modules allocation related scripts (default multipleAccess) fo…
2d35d3f 
…r multiple modules feature
@aderiabin
Add RC modules allocation related scripts (no moduleId) for multiple …
459e524 
…modules feature
@aderiabin
Add RC modules releasing related scripts for multiple modules feature
f7d2781
@aderiabin
Add GetInteriorVehicleDataConsent RPC related scripts for multiple mo…
a47328c 
…dules feature
@aderiabin
Add SetGlobalProperties RPC (user location) related scripts for multi…
276e4ad 
…ple modules feature
@aderiabin
Add driver decision caching related scripts for multiple modules feature
738e731
@aderiabin
Add test set for multiple modules feature
258c973
@aderiabin
Add tests for RC related RPCs into smoke test set
e354d13
@aderiabin
Update existing scripts according multiple modules feature changes
aad7bf3
@aderiabin aderiabin force-pushed the feature/multiple_modules branch from 44a50fa to aad7bf3 Compare August 19, 2019 09:36
@aderiabin
Copy link
Contributor

Rebased on the latest develop b2d70d1

@mrapitis
Copy link

@theresalech Ford has approved of these changes.

@theresalech theresalech requested a review from ShobhitAd August 19, 2019 18:42
@LitvinenkoIra
Copy link
Contributor Author

According to clarifications during review of the core:
smartdevicelink/sdl_core#2984 (comment)
smartdevicelink/sdl_core#2984 (comment)
smartdevicelink/sdl_core#2984 (comment)

Appropriate changes in sdl atf test scripts were made - #2250
and merged to the feature branch

@IGapchuk
Remove non actual script.
31a971a 
According to new SDL Core's behavior the script is not actual.
@IGapchuk
Copy link
Contributor

IGapchuk commented Sep 2, 2019

According to clarifications during review of the core:

smartdevicelink/sdl_core#2984 (comment)
smartdevicelink/sdl_core#2984 (comment)
smartdevicelink/sdl_core#2984 (comment)

has been removed (#2253) non actual script in sdl atf test scripts repository and these changes have been merged into feature branch.

@dboltovskyi
Copy link
Contributor

Please notice there was one scripts' instability resolved in 9d76c49

iCollin
iCollin reviewed Sep 3, 2019
View reviewed changes
Copy link
Collaborator

@iCollin iCollin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes needed to remove consequentially. I made suggestions where I believe sequentially was meant, but the wording in ModulesAllocation/001 - ModulesAllocation/004 will also need to be adjusted.

...ltipleModules/ModulesAllocation/058_GetInteriorVehicleDataConsent_all_correct_one_module.lua Outdated Show resolved Hide resolved
...ltipleModules/ModulesAllocation/058_GetInteriorVehicleDataConsent_all_correct_one_module.lua Outdated Show resolved Hide resolved
...pleModules/ModulesAllocation/059_1_GetInteriorVehicleDataConsent_ASK_DRIVER_free_modules.lua Outdated Show resolved Hide resolved
...pleModules/ModulesAllocation/059_1_GetInteriorVehicleDataConsent_ASK_DRIVER_free_modules.lua Outdated Show resolved Hide resolved
.../MultipleModules/ModulesAllocation/059_2_GetInteriorVehicleDataConsent_ASK_DRIVER_cached.lua Outdated Show resolved Hide resolved
...ltipleModules/ModulesAllocation/073_GetInteriorVehicleDataConsent_AUTO_DENY_free_modules.lua Outdated Show resolved Hide resolved
...ulesAllocation/074_GetInteriorVehicleDataConsent_AUTO_DENY_multiple_modules_part_allowed.lua Outdated Show resolved Hide resolved
...ulesAllocation/074_GetInteriorVehicleDataConsent_AUTO_DENY_multiple_modules_part_allowed.lua Outdated Show resolved Hide resolved
...ltipleModules/ModulesAllocation/077_SetGlobalProperties_has_no_influence_on_getting_data.lua Outdated Show resolved Hide resolved
...ltipleModules/ModulesAllocation/077_SetGlobalProperties_has_no_influence_on_getting_data.lua Outdated Show resolved Hide resolved
@iCollin
Copy link
Collaborator

iCollin commented Sep 3, 2019 •
edited
Loading

Why is there no test ModulesAllocation/067? (no big deal but I did see other renames)
Why are ModulesAllocation/59_1 - ModulesAllocation/59_4 not included in test_sets/rc_allow_multiple_modules_per_module_type.txt?

@iCollin
Copy link
Collaborator

iCollin commented Sep 3, 2019 •
edited
Loading

Please confirm that everywhere you wrote without asking driver there is no GetInteriorVehicleDataConsent RPC sent to the HMI.

@iCollin
Copy link
Collaborator

iCollin commented Sep 3, 2019

We may be seeing an issue with accessMode=ASK_DRIVER where the consent is sometimes not being added to app_info.dat.
Could we add on to the Cache tests a check that consent is added to app_info.dat?

@aderiabin
Copy link
Contributor

aderiabin commented Sep 4, 2019 •
edited
Loading

@iCollin

Changes needed to remove consequentially. I made suggestions where I believe sequentially was meant, but the wording in ModulesAllocation/001 - ModulesAllocation/004 will also need to be adjusted.

Corrected in all scripts (see commit 61b67ae)

Why is there no test ModulesAllocation/067? (no big deal but I did see other renames)

Test ModulesAllocation/067 was removed as not actual according next changes which have been made in core during its review:
smartdevicelink/sdl_core#2984 (comment)
smartdevicelink/sdl_core#2984 (comment)
smartdevicelink/sdl_core#2984 (comment)

Why are ModulesAllocation/59_1 - ModulesAllocation/59_4 not included in test_sets/rc_allow_multiple_modules_per_module_type.txt?

Mentioned tests have been added to the test set (see commit cb9d8fa)

Please confirm that everywhere you wrote without asking driver there is no GetInteriorVehicleDataConsent RPC sent to the HMI.

Currently it is not. According last changes which have been made in core during its review:
smartdevicelink/sdl_core#2984 (comment)
smartdevicelink/sdl_core#2984 (comment)
smartdevicelink/sdl_core#2984 (comment)
GetInteriorVehicleDataConsent RPC is sent to HMI only if are passed next conditions:

  • allowMultipleAccess is true for requested module
  • user location of mobile application is within service area of requested module or it is driver location
  • RC access mode is ASK_DRIVER
  • requested module is currently allocated to another application

So, the descriptions of test steps have been updated: without asking driver has been removed for each such step description. (see commit 4cb649a)

We may be seeing an issue with accessMode=ASK_DRIVER where the consent is sometimes not being added to app_info.dat.
Could we add on to the Cache tests a check that consent is added to app_info.dat?

Currently ATF tests aimed on performing black box testing and do not checks SDL inner implementation artifacts (like policy.sqlite or app_info.dat,) because SDL manage its artifacts in specific way storing data on disk not immediately . So test scripts check a fact of caching of driver decisions by requesting module reallocation for a some time and in special conditions (unregistration of application and IGNITION OFF) after GetInteriorVehicleDataConsent RPC has been processed and driver has allowed/disallowed reallocation of a module. (see Cache test cases)

But we have checked functionality of saving driver decisions to app_info.dat file by unit tests for RCConsentManager (see https://github.com/smartdevicelink/sdl_core/blob/develop/src/components/application_manager/rpc_plugins/rc_rpc_plugin/test/rc_consent_manager_impl_test.cc)

By the way I tried to discover instability in caching functionality running Multiple Modules/Cache ATF tests in loop, but have not faced it.

@iCollin iCollin merged commit 118faaa into develop Sep 4, 2019
@LitvinenkoIra LitvinenkoIra deleted the feature/multiple_modules branch September 6, 2019 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iCollin iCollin iCollin left review comments

@aderiabin aderiabin aderiabin left review comments

@dboltovskyi dboltovskyi dboltovskyi left review comments

@mrapitis mrapitis mrapitis approved these changes

@Jack-Byrne Jack-Byrne Jack-Byrne approved these changes

@ShobhitAd ShobhitAd Awaiting requested review from ShobhitAd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@LitvinenkoIra @aderiabin @mrapitis @IGapchuk @dboltovskyi @iCollin @Jack-Byrne @GetmanetsIrina