Support set_gsm_signal #357

ki4070ma · 2019-04-29T16:33:20Z

ki4070ma · 2019-04-29T16:40:21Z

appium/webdriver/extensions/gsm.py

+        MODERATE,
+        GOOD,
+        GREAT
+    ) = list(map(str, range(5)))  # TODO: Need to check where int 0 will be dropped as arg


Here is log when int 0, not '0', is used.
4 is used instead of 0 in adb execution.

[HTTP] --> POST /wd/hub/session/806d31f9-84d0-4bd4-8264-cd6af8dd7ebb/appium/device/gsm_signal (★) [HTTP] {"signalStrength":0,"sessionId":"806d31f9-84d0-4bd4-8264-cd6af8dd7ebb"} [W3C (806d31f9)] Calling AppiumDriver.gsmSignal() with args: [null,"806d31f9-84d0-4bd4-8264-cd6af8dd7ebb"] [ADB] gsm signal-profile changes the reported strength on next (15s) update. [ADB] Getting connected emulators [ADB] Getting connected devices... [ADB] 1 device(s) connected [ADB] 1 emulator(s) connected (★)[ADB] Running '/Users/atsushimori/Library/Android/sdk/platform-tools/adb -P 5037 -s emulator-5554 emu gsm signal-profile 4'

the 4 is the default value
https://github.com/appium/appium-adb/blob/527c8b417d7b62717ececfdf068309b0ee4dbd8f/lib/tools/adb-emu-commands.js#L165

Yes, I've checked.
It's problem that 4(Gsm.GREAT) is set on the device when self.driver.set_gsm_signal(0)(Gsm.NONE_OR_UNKNOWN) is called. But self.driver.set_gsm_signal('0') works.

I'll look into appium-adb codes more.

Your [W3C (806d31f9)] Calling AppiumDriver.gsmSignal() with args: [null,"806d31f9-84d0-4bd4-8264-cd6af8dd7ebb"] should be like below when you send the request to server.
null is the cause of using 4 which is the default value.

[debug] [W3C (0df92b78)] Calling AppiumDriver.gsmSignal() with args: [0,"0df92b78-87ce-441e-93c7-2baf70eef29d"]

Thanks. Hum, I'll keep current implementation and remove my comments.

string "0" works.

[HTTP] --> POST /wd/hub/session/604c1c76-bb99-4e47-b998-7ba2590a9fa6/appium/device/gsm_signal (★)[HTTP] {"signalStrengh":"0","sessionId":"604c1c76-bb99-4e47-b998-7ba2590a9fa6"} [W3C (604c1c76)] Calling AppiumDriver.gsmSignal() with args: ["0","604c1c76-bb99-4e47-b998-7ba2590a9fa6"] [ADB] gsm signal-profile changes the reported strength on next (15s) update. [ADB] Getting connected emulators [ADB] Getting connected devices... [ADB] 1 device(s) connected [ADB] 1 emulator(s) connected (★)[ADB] Running '/Users/atsushimori/Library/Android/sdk/platform-tools/adb -P 5037 -s emulator-5554 emu gsm signal-profile 0'

I wonder Python client has an issue to filter 0.
Could you add a unit test as the zero value?

Mine was by Ruby client. If JSON request has 0, Appium should work.

Just my guess is due to python spec like below.
I'll look into codes more.

➜ mobile git:(master) ✗ python Python 3.7.1 (default, Dec 29 2018, 23:35:51) >>> if 0: ... print('hoge') ... >>> if 1: ... print('hoge') ... hoge

so far, https://github.com/appium/python-client/pull/357/files#r279606290 is needed. I found an issue.
Then, 0 should work

Thanks for your info, it works.

KazuCocoa · 2019-04-30T03:41:31Z

appium/webdriver/extensions/gsm.py

+        MODERATE,
+        GOOD,
+        GREAT
+    ) = list(map(str, range(5)))  # TODO: Need to check where int 0 will be dropped as arg


the 4 is the default value
https://github.com/appium/appium-adb/blob/527c8b417d7b62717ececfdf068309b0ee4dbd8f/lib/tools/adb-emu-commands.js#L165

KazuCocoa · 2019-04-30T03:43:28Z

appium/webdriver/extensions/gsm.py

+        :Usage:
+            self.driver.set_gsm_signal(Gsm.GOOD)
+        """
+        self.execute(Command.SET_GSM_SIGNAL, {'signalStrength': strength})


Which Appium version did you run on your local?
We should send two params here because of appium/appium-base-driver#308

{'signalStrength': strength,'signalStrengh': strength})

Which Appium version did you run on your local?

v1.12.1

Thanks for your notice, I'll support both params.

@KazuCocoa

We should send two params here because of appium/appium-base-driver#308

Let me confirm, this reason is to cover old appium case, right?
(If so, I'll check another client behavior like java.)

appium v1.10 (accept only "signalStrengh")

appium v1.11+ (accept both "signalStrengh" and "signalStrength")

I'm having below error log when sending {'signalStrength': strength,'signalStrengh': strength}) to appium v1.10.
I can send {'signalStrength': strength,'signalStrengh': strength}) to appium v1.12.1, but I think {'signalStrength': strength}) is enough in this case.

➜ mobile git:(master) ✗ python SampleAndroidTest.py test_01_test_name (__main__.SimpleTest) ... 0 ERROR ====================================================================== ERROR: test_01_test_name (__main__.SimpleTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "SampleAndroidTest.py", line 42, in test_01_test_name self.driver.set_gsm_signal(Gsm.POOR) File "/Users/atsushimori/.pyenv/versions/3.7.1/lib/python3.7/site-packages/appium/webdriver/extensions/gsm.py", line 38, in set_gsm_signal self.execute(Command.SET_GSM_SIGNAL, {'signalStrength': strength, 'signalStrengh': strength}) File "/Users/atsushimori/.pyenv/versions/3.7.1/lib/python3.7/site-packages/selenium/webdriver/remote/webdriver.py", line 321, in execute self.error_handler.check_response(response) File "/Users/atsushimori/.pyenv/versions/3.7.1/lib/python3.7/site-packages/appium/webdriver/errorhandler.py", line 29, in check_response raise wde File "/Users/atsushimori/.pyenv/versions/3.7.1/lib/python3.7/site-packages/appium/webdriver/errorhandler.py", line 24, in check_response super(MobileErrorHandler, self).check_response(response) File "/Users/atsushimori/.pyenv/versions/3.7.1/lib/python3.7/site-packages/selenium/webdriver/remote/errorhandler.py", line 242, in check_response raise exception_class(message, screen, stacktrace) selenium.common.exceptions.InvalidArgumentException: Message: Parameters were incorrect. We wanted {"required":["signalStrengh"]} and you sent ["signalStrength","signalStrengh","sessionId"] ---------------------------------------------------------------------- Ran 1 test in 22.366s FAILED (errors=1)

ah, yeah. The current way looks good.
Thanks for it.

Changed to {'signalStrength': strength,'signalStrengh': strength}) for #357 (comment)

KazuCocoa · 2019-04-30T13:08:01Z

appium/webdriver/extensions/gsm.py

+        :Usage:
+            self.driver.set_gsm_signal(Gsm.GOOD)
+        """
+        self.execute(Command.SET_GSM_SIGNAL, {'signalStrength': strength, 'signalStrengh': strength})


What about returning an error message if a user selects out of https://github.com/appium/python-client/pull/357/files#diff-bb5916ee1320d1333f6c9ba19cf04705R22 ?

if strength not in [NONE_OR_UNKNOWN, .....]: return 'error message'

Thanks, seems good.
If throwing exception is too enough, please let me know.

KazuCocoa

thanks

Since arg validation already done by server side

KazuCocoa · 2019-05-01T01:48:39Z

appium/webdriver/extensions/gsm.py

        :Usage:
            self.driver.set_gsm_signal(Gsm.GOOD)
        """
-        if strength not in [self.NONE_OR_UNKNOWN, self.POOR, self.MODERATE, self.GOOD, self.GREAT]:


Here is pretty good to show users which args can use for this method as either warn or error
(Here is client-side logic matter)

Added warning log instead of throwing exception.
Here is behavior.

➜ mobile git:(master) ✗ python SampleAndroidTest.py test_01_test_name (__main__.SimpleTest) ... 100 is out of range. Use the value like Gsm.GOOD for signal strength. ERROR ====================================================================== ERROR: test_01_test_name (__main__.SimpleTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "SampleAndroidTest.py", line 48, in test_01_test_name self.driver.set_gsm_signal(100) File "/Users/atsushimori/.pyenv/versions/3.7.1/lib/python3.7/site-packages/appium/webdriver/extensions/gsm.py", line 41, in set_gsm_signal self.execute(Command.SET_GSM_SIGNAL, {'signalStrength': strength, 'signalStrengh': strength}) File "/Users/atsushimori/.pyenv/versions/3.7.1/lib/python3.7/site-packages/selenium/webdriver/remote/webdriver.py", line 321, in execute self.error_handler.check_response(response) File "/Users/atsushimori/.pyenv/versions/3.7.1/lib/python3.7/site-packages/appium/webdriver/errorhandler.py", line 29, in check_response raise wde File "/Users/atsushimori/.pyenv/versions/3.7.1/lib/python3.7/site-packages/appium/webdriver/errorhandler.py", line 24, in check_response super(MobileErrorHandler, self).check_response(response) File "/Users/atsushimori/.pyenv/versions/3.7.1/lib/python3.7/site-packages/selenium/webdriver/remote/errorhandler.py", line 242, in check_response raise exception_class(message, screen, stacktrace) selenium.common.exceptions.WebDriverException: Message: An unknown server-side error occurred while processing the command. Original error: Invalid signal strength param 100. Supported values: 0,1,2,3,4 ---------------------------------------------------------------------- Ran 1 test in 20.444s FAILED (errors=1)

[nits]
what about something like {name for name, value in vars(self).items() if name.isupper()} to show the consts instead of the number?

I misunderstood for 1ca58f4

selenium.common.exceptions.WebDriverException: Message: An unknown server-side error occurred while processing the command. Original error: Invalid signal strength param 100. Supported values: 0,1,2,3,4

This log comes from https://github.com/appium/appium-adb/blob/527c8b417d7b62717ececfdf068309b0ee4dbd8f/lib/tools/adb-emu-commands.js#L168 .
So can't handle from client side.

Added log is here.

➜ mobile git:(master) ✗ python SampleAndroidTest.py test_01_test_name (__main__.SimpleTest) ... 100 is out of range. Use the value in ['Gsm.NONE_OR_UNKNOWN', 'Gsm.POOR', 'Gsm.MODERATE', 'Gsm.GOOD', 'Gsm.GREAT']. ERROR (--snip--)

Ah, okay. I misunderstood it was the warning in this pr

KazuCocoa

Looks good to me

ki4070ma · 2019-05-01T07:01:26Z

Will rename gsm.py, Gsm to gsm_signal, GsmSignal in the next pr because gsm.py, Gsm can't cover GsmVoice, GsmCall with current style.

ki4070ma added 3 commits April 30, 2019 00:44

Support set_gsm_signal

fc441e6

Fix: NONE_OR_UNKNOWN doesn't work

1b0f797

Add set_gsm_signal unittest

73f35c0

ki4070ma commented Apr 29, 2019

View reviewed changes

KazuCocoa requested changes Apr 30, 2019

View reviewed changes

Use int for signal strength const

a9f63d5

ki4070ma requested a review from KazuCocoa April 30, 2019 10:55

KazuCocoa mentioned this pull request Apr 30, 2019

fix handling 0 in gsmSignal appium/appium-base-driver#321

Merged

KazuCocoa reviewed Apr 30, 2019

View reviewed changes

ki4070ma added 2 commits April 30, 2019 23:23

Raise exception when signal strength is out of range

ddc93cb

Fix: wrong class name

5a03d31

mykola-mokhnach approved these changes Apr 30, 2019

View reviewed changes

KazuCocoa approved these changes Apr 30, 2019

View reviewed changes

Removed args validation

bbbb9df

Since arg validation already done by server side

KazuCocoa reviewed May 1, 2019

View reviewed changes

ki4070ma added 2 commits May 1, 2019 12:48

Show warning log when arg is out of range

c2ee455

Some changes for less maintenance

1ca58f4

KazuCocoa approved these changes May 1, 2019

View reviewed changes

ki4070ma merged commit 308fd1b into appium:master May 1, 2019

ki4070ma deleted the gsm branch May 1, 2019 11:33

Support set_gsm_signal #357

Support set_gsm_signal #357

Uh oh!

Conversation

ki4070ma commented Apr 29, 2019

Uh oh!

ki4070ma Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ki4070ma Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ki4070ma Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KazuCocoa Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ki4070ma Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KazuCocoa left a comment

Choose a reason for hiding this comment

Uh oh!

KazuCocoa May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KazuCocoa May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KazuCocoa left a comment

Choose a reason for hiding this comment

Uh oh!

ki4070ma commented May 1, 2019

Uh oh!

Uh oh!

ki4070ma Apr 29, 2019 •

edited

Loading

ki4070ma Apr 30, 2019 •

edited

Loading

ki4070ma Apr 30, 2019 •

edited

Loading

KazuCocoa Apr 30, 2019 •

edited

Loading

ki4070ma Apr 30, 2019 •

edited

Loading

KazuCocoa May 1, 2019 •

edited

Loading

KazuCocoa May 1, 2019 •

edited

Loading