949646 - Improve Eideticker b2g device/network management

Status: RESOLVED FIXED

(Testing Graveyard :: Eideticker, defect)

Description

[William Lachance (:wlach)]

There's much weird code in here dating back to the time that we were trying to get Eideticker working with the Panda last year. I propose the following:

1. Stop using mozb2g. We can inline the little functionality from that module that we actually need.
2. Use GaiaTest's method of connecting to WIFI and make it possible to specify wifi settings as a command line argument. This eliminates the need to setup the device's networking before running Eideticker. At the same time, only connect to WIFI if actually needed (currently this is only when we're synchronizing time with the machine running the test for action logging purposes). This will make it easier to test Eideticker in environments where we don't have a shared wireless network handy.
3. Kill the various cleanup stuff -- it shouldn't be required.

Comment 1

[William Lachance (:wlach)]

Attachment: Clean up B2G network/device management;r=davehunt

* Stop using mozb2g
* Connect to network only when required, using pre-specified wifi settings
* Don't clean up device

Comment 2

[William Lachance (:wlach)]

Attachment: Clean up B2G network/device management;r=davehunt

* Stop using mozb2g
* Connect to network only when required, using pre-specified wifi settings
* Don't clean up device

Comment 3

[William Lachance (:wlach)]

Comment on attachment 8346763 [details] [diff] [review]
Clean up B2G network/device management;r=davehunt

This patch had some problems, chief among them getdimensions.py wasn't updated.

Comment 4

[William Lachance (:wlach)]

Comment on attachment 8346768 [details] [diff] [review]
Clean up B2G network/device management;r=davehunt

Review of attachment 8346768 [details] [diff] [review]:
-----------------------------------------------------------------

This one should work better.

Comment 5

[William Lachance (:wlach)]

Attachment: 0001-Bug-949646-Clean-up-B2G-network-device-management-r-.patch

Previous patch had some problems too. This one seems to work well, I swear. ;)

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8346781 [details] [diff] [review]
0001-Bug-949646-Clean-up-B2G-network-device-management-r-.patch

Review of attachment 8346781 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking great! Most of my comments are not essential to address.

::: bin/create-wifi-settings-file.py
@@ +1,1 @@
> +#!/usr/bin/env python

This file isn't executable for me.

@@ +3,5 @@
> +import json
> +import sys
> +
> +if len(sys.argv) != 2 and len(sys.argv) != 4:
> +    print "USAGE: %s <SSID> [SECURITY MODEL (WEP or WPA-PSK)] [PASSWORD]"

Should we exit here?

@@ +5,5 @@
> +
> +if len(sys.argv) != 2 and len(sys.argv) != 4:
> +    print "USAGE: %s <SSID> [SECURITY MODEL (WEP or WPA-PSK)] [PASSWORD]"
> +
> +network = { 'ssid': sys.argv[1] }

Fails if no arguments are passed.

@@ +8,5 @@
> +
> +network = { 'ssid': sys.argv[1] }
> +if len(sys.argv) == 4:
> +    network['keyManagement'] = sys.argv[2]
> +    network['psk'] = sys.argv[3]

If keyManagement is WEP then this should be wep and not wpa. See https://github.com/mozilla-b2g/gaia/blob/34c8e31c0d406486a46479a2700b4ac58581ea3b/tests/python/gaia-ui-tests/gaiatest/gcli.py#L148

::: src/eideticker/eideticker/device.py
@@ +380,5 @@
>      """B2G-specific extensions to the eideticker mixin"""
>  
> +    marionette = None
> +
> +    def waitForMarionettePort(self):

If we need this, could we call marionette.wait_for_port instead?

@@ +402,5 @@
> +
> +    def setupMarionette(self):
> +        self.marionette = marionette.Marionette()
> +        self._logger.info("Waiting for Marionette...")
> +        self.waitForMarionettePort()

Is this needed? We don't do this in the Gaia functional UI tests. If we're using this to determine when B2G has started then it might be problematic. We've had problems before when proceeding before the homescreen or FTU apps are finished loading.

@@ +417,5 @@
> +        """
> +        data = GaiaData(self.marionette)
> +        data.connect_to_wifi(wifiSettings)
> +
> +        # Wait for device to properly recognize network

This shouldn't be needed now, the connect_to_wifi won't return until the connection is established.

@@ -409,4 @@
>      def rotation(self):
>          return 90 # Assume portrait orientation for now
>  
> -class B2GSUT(EidetickerB2GMixin, mozb2g.DeviceSUT):

Do we not need to support SUT?

::: src/eideticker/eideticker/options.py
@@ +30,5 @@
>                          "android). If B2G, you do not need to pass in an "
>                          "appname")
> +        self.add_option("-w", "--wifi-settings", action="store",
> +                        type = "string", dest = "wifi_settings_file",
> +                        default = os.environ.get('WIFI_SETTINGS'),

I don't think we need the environment variable default. This might be useful if the variable could contain the settings, but as a path to the file I don't think it will be too useful. I'm okay with piping the output from the create script to file in the CI.

::: src/eideticker/eideticker/runtest.py
@@ +48,5 @@
> +        # restart b2g, so we're in a clean state
> +        device.restartB2G()
> +
> +        if sync_time:
> +            # if we're synchronizing time, we need to connect to the network

Great to do this only if needed, otherwise it wastes time. Another advantage might be that we don't see the update notification (unless device has cell data, but I believe that's disabled by default).

Comment 7

[William Lachance (:wlach)]

Agree with most of this (have a patch that I'm about to upload)

(In reply to Dave Hunt (:davehunt) from comment #6)
> @@ -409,4 @@
> >      def rotation(self):
> >          return 90 # Assume portrait orientation for now
> >  
> > -class B2GSUT(EidetickerB2GMixin, mozb2g.DeviceSUT):
> 
> Do we not need to support SUT?

No. SUT never really worked on B2G. We'll stick with ADB on this platform.

(I would argue we should migrate to using ADB for our Android testing as well, but that's another can of worms)

Comment 8

[William Lachance (:wlach)]

Attachment: 0001-Bug-949646-Clean-up-B2G-network-device-management-r-.patch

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8347416 [details] [diff] [review]
0001-Bug-949646-Clean-up-B2G-network-device-management-r-.patch

Review of attachment 8347416 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great!

::: src/eideticker/eideticker/runtest.py
@@ +47,5 @@
>      if device_prefs['devicetype'] == 'b2g':
> +        # restart b2g, so we're in a clean state
> +        device.restartB2G()
> +
> +        device.marionette.execute_async_script("""

You could put this into restartB2G as it should be necessary whenever restarting to ensure we wait for B2G to finish booting up.

Comment 10

[William Lachance (:wlach)]

(In reply to Dave Hunt (:davehunt) from comment #9)
> Comment on attachment 8347416 [details] [diff] [review]
> ::: src/eideticker/eideticker/runtest.py
> @@ +47,5 @@
> >      if device_prefs['devicetype'] == 'b2g':
> > +        # restart b2g, so we're in a clean state
> > +        device.restartB2G()
> > +
> > +        device.marionette.execute_async_script("""
> 
> You could put this into restartB2G as it should be necessary whenever
> restarting to ensure we wait for B2G to finish booting up.

Good point. Fixed.

Comment 11

[William Lachance (:wlach)]

https://github.com/mozilla/eideticker/commit/9768faf3ebdfcf89779d1022c3d348e0df9c88e2