Skip to content

feat: add env_str to snippet_client_v2.Config to allow am instrument command prefix (fixes #1004)#1008

Closed
devimano2011 wants to merge 3 commits intogoogle:masterfrom
devimano2011:feat/add-env-str-snippet-client-v2-1004
Closed

feat: add env_str to snippet_client_v2.Config to allow am instrument command prefix (fixes #1004)#1008
devimano2011 wants to merge 3 commits intogoogle:masterfrom
devimano2011:feat/add-env-str-snippet-client-v2-1004

Conversation

@devimano2011
Copy link
Copy Markdown
Contributor

Fixes #1004

Problem

Users who need to prepend environment variables or command wrappers (e.g. ENV_VAR=value or a wrapper command) to the am instrument command in snippet_client_v2 had no way to do so.

Solution

Add an env_str field to snippet_client_v2.Config that, when set, is prepended to the am instrument command. This allows users to:

  • Set environment variables: config.env_str = 'MY_VAR=value'
    • Use command wrappers: config.env_str = 'wrapper_cmd'

Changes

  • Added env_str: str = '' field to the Config dataclass with documentation
    • Updated _LAUNCH_CMD template to support {env_str} prefix
    • Pass env_str value when building the launch command in _start_server()

Usage

config = snippet_client_v2.Config(env_str='MY_VAR=hello')
ad.load_snippet('maps', 'com.example.maps', config=config)

fixes google#1004)

Add optional environment variable string to launch command.
Copy link
Copy Markdown
Collaborator

@mhaoli mhaoli left a comment

Choose a reason for hiding this comment

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

Please add unit tests for the changes.

Add 5 unit tests covering the env_str field added to snippet_client_v2.Config:
- test_config_env_str_default_is_empty_string
- test_config_env_str_can_be_set
- test_start_server_with_env_str
- test_start_server_without_env_str_no_extra_space
- test_start_server_with_env_str_has_trailing_space
- test_start_server_with_multiple_env_vars
@devimano2011
Copy link
Copy Markdown
Contributor Author

Hi @mhaoli, unit tests have been added as requested in commit d2dd8fa.

The following 6 tests were added to tests/mobly/controllers/android_device_lib/snippet_client_v2_test.py:

  • test_config_env_str_default_is_empty_string — verifies Config.env_str defaults to ''
    • test_config_env_str_can_be_set — verifies the field stores an assigned value correctly
    • test_start_server_with_env_str — verifies a non-empty env_str is prepended to the am instrument command
    • test_start_server_without_env_str_no_extra_space — verifies no extra whitespace is injected when env_str is empty
    • test_start_server_with_env_str_has_trailing_space — verifies exactly one space separates env_str from am instrument
    • test_start_server_with_multiple_env_vars — verifies multiple space-separated env vars are preserved
      Please let me know if any further changes are needed. Thanks!

# Tests for Config.env_str (PR #1008)
# ---------------------------------------------------------------------------

def test_config_env_str_default_is_empty_string(self):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why are you adding unit tests under the if __name__ == '__main__' block?

@devimano2011
Copy link
Copy Markdown
Contributor Author

Good catch @mhaoli! The tests were accidentally placed inside the if __name__ == '__main__' block. Fixed in the latest commit — all 6 tests are now correctly at class level. Apologies for the error!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants