Fix AndroidDevice#take_screenshot failure caused by bracket characters
#986
Conversation
|
Debug tag shouldn't contain slashes in the first place, no?
Why does a debug tag need to contain "/"? It'll cause all kinds of problems
in other places
…On Mon, Aug 11, 2025, 03:17 Minghao Li ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/mobly/controllers/android_device_test.py
<#986 (comment)>:
> @@ -1301,6 +1348,64 @@ def custom_shell_for_screenshot(params, timeout=None):
],
)
+ @mock.patch(
+ 'mobly.controllers.android_device_lib.adb.AdbProxy',
+ return_value=mock_android_device.MockAdbProxy('1'),
+ )
+ @mock.patch(
+ 'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
+ return_value=mock_android_device.MockFastbootProxy('1'),
+ )
+ @mock.patch('mobly.utils.create_dir')
+ @mock.patch('mobly.logger.get_log_file_timestamp')
+ def test_AndroidDevice_take_screenshot_all_displays_add_double_quotations(
Modified, PTAL, thanks.
The use case is that current implementation fails when debug_tag contains
"(" / ")" character.
—
Reply to this email directly, view it on GitHub
<#986 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARDNZKC2HACWMUHKAFTVTL3NBUSBAVCNFSM6AAAAACDOHYBCOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTCMBVGEZDANBQGQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Sorry my previous reply looks confusing in email. The problem is not due to slashes, but brackets. E.g., my test sets debug tag to Also, this does not cause failure for most mobly provided APIs, I only noticed failure in |
|
Ok, a test should cover this actual case instead? |
mhaoli
changed the title
AndroidDevice#take_screenshot failure when debug tag contains bracket characters
|
Do you mean add an integration test case in internal codebase? |
mhaoli
changed the title
AndroidDevice#take_screenshot failure when debug tag contains bracket charactersAndroidDevice#take_screenshot failure caused by bracket characters
|
Now we have unit tests for this CUJ. I'm creating the CL for a integration test case internally. |
There was a problem hiding this comment.
If path validation is common across the code base, should we utilize https://pypi.org/project/pathvalidate/ ?
| return [ad.serial for ad in get_mock_ads(5)] | ||
|
|
||
|
|
||
| def _assert_valid_path_in_adb_shell_cmd(path: str): |
There was a problem hiding this comment.
I don't see anything specific to "adb shell" in this function.
Also this seems like an overkill, with many questionable checks...
Might be more straightforward to perform the actual check needed in the test case instead.
| ) | ||
| elif 'which' in params: | ||
| return b'' | ||
| elif isinstance(params, list) and params and params[0] == 'rm': |
There was a problem hiding this comment.
this is too much logic
I don't think anybody else could understand and maintain these effectively.
consider doing the proper validation in each test case.
2 participants
API
AndroidDevice#take_screenshotfails when debug tag contains bracket characters.The root cause is that
adb shelldo not automatically handle special characters like brackets. So this PR adds double quotations to args ofadb.shellcalls.Bracket characters do not affect most Mobly provided APIs, e.g. calling
adb.pullwith bracket characters works well.Tested locally.
This change is