Closed Bug 821412 Opened 11 years ago Closed 11 years ago

B2G Updates: New Marionette based frontend for automated update tests

Categories

(Remote Protocol :: Marionette, defect)

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

RESOLVED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: marshall, Assigned: marshall)

References

Details

Attachments

(5 files, 4 obsolete files)

This new frontend will try to solve a few problems:

- Marionette test cases are currently written with the assumption that they are self-contained in a single gecko instance. To fully automate the updater, we need to support test cases that may span restarts of the b2g process (OTA), or device reboots (FOTA)

- Many updater problems are intermittent, and we need automation to help us mitigate and catch these issues. This frontend should provide automation hooks so we can plug in actually generated MARs and images, and do automated smoketesting to check for regressions.

- Currently, the only way for developers to actually unit test the update system in B2G is through xpcshell tests that only work on Desktop. The majority of the B2G update special sauce actually lives in Gonk, which requires us to do update on device. This frontend should also provide a working interface for developer unit testing of updates in B2G.
Attached patch part 1: marionette changes - v1 (obsolete) — Splinter Review
Marionette changes to support the new update test frontend
Attachment #691926 - Flags: review?(jgriffin)
Attachment #691926 - Flags: review?(jgriffin) → feedback?(jgriffin)
Attachment #691927 - Flags: feedback?(jgriffin)
Attachment #691929 - Flags: feedback?(netzen)
Attachment #691929 - Flags: feedback?(jones.chris.g)
Attachment #691929 - Flags: feedback?(jgriffin)
Attachment #691929 - Flags: feedback?(jones.chris.g) → feedback+
Attachment #691959 - Flags: feedback?(mwu)
Attachment #691959 - Flags: feedback?(dhylands)
blocking-basecamp: --- → ?
Attachment #691929 - Flags: feedback?(netzen) → feedback+
Comment on attachment 691959 [details] [diff] [review]
part 4: update_tools and test.sh support - v1

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

Just a couple minor comments

::: scripts/updates.sh
@@ +1,4 @@
> +#!/bin/bash
> +
> +# Determine the absolute path of our location.
> +B2G_DIR=$(cd `dirname $0`/..; pwd)

nit: I can go either way, but I personally prefer $(dirname $0) over the backtick variety.

@@ +28,5 @@
> +MARIONETTE_TESTS=${MARIONETTE_TESTS:-$TEST_PATH}
> +echo "Running tests: $MARIONETTE_TESTS"
> +
> +SCRIPT=$GECKO_PATH/testing/marionette/client/marionette/venv_b2g_update_test.sh
> +PYTHON=${PYTHON:-`which python`}

This should probably be done using type instead of which (type is a builtin, which is usually some external utility or alias)

PYTHON=${PYTHON:-$(type -p python)}
Attachment #691959 - Flags: feedback?(dhylands) → feedback+
Comment on attachment 691926 [details] [diff] [review]
part 1: marionette changes - v1

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

Cool!  Eventually we'll want to refactor how this code, other parts of Marionette, mozb2g, mozdevice fit together, but I'm totally fine with kicking that can down the road for now.

::: testing/marionette/client/marionette/b2ginstance.py
@@ +65,5 @@
> +        """Import the update_tools package from B2G"""
> +        sys.path.append(self.update_tools)
> +        import update_tools
> +        sys.path.pop()
> +        return update_tools

Handling the packaging this way can potentially cause version mismatches, since this code will rely on an unversioned Python file in a different repo.  It might be better to actually put the update_tools in a real versioned Python package, and then spin this code off into a Marionette subclass which could depend on that package in setup.py.  But, I'm ok with landing this as-is and working out packaging details later.

@@ +71,5 @@
> +    def flash(self, erase=None, max_retries=5, **images):
> +        erase = erase or ()
> +        if len(erase) == 0 and len(images) == 0:
> +            print 'Warning: flash called with no images or partitions to erase.'
> +            return

Shouldn't we raise an exception here?

@@ +113,5 @@
> +
> +        try:
> +            adb.run('wait-for-device')
> +        except:
> +            # swallow the harmless "more than one device / emulator" error

It's not exactly harmless, since if that error occurs, it will mean we don't end up waiting for the device.  Does the 'more than one device' error occur during normal flashing, when you only have one device hooked up?  I haven't seen it locally.
Attachment #691926 - Flags: feedback?(jgriffin) → feedback+
(In reply to Jonathan Griffin (:jgriffin) from comment #6)
> @@ +113,5 @@
> > +
> > +        try:
> > +            adb.run('wait-for-device')
> > +        except:
> > +            # swallow the harmless "more than one device / emulator" error
> 
> It's not exactly harmless, since if that error occurs, it will mean we don't
> end up waiting for the device.  Does the 'more than one device' error occur
> during normal flashing, when you only have one device hooked up?  I haven't
> seen it locally.

This may be a bootup oddity, but I see the error regularly with only 1 device after "adb wait-for-device" returns from a freshly booted unagi. From my terminal, it looks like this:

% adb wait-for-device; print error_code=$?; adb devices
[plug in a booting unagi, wait a while...]
error: more than one device and emulator
error_code=1
List of devices attached 
full_unagi	device

Probably the right way to fix this is to pass on the --device serial if it is supplied to Marionette, that way there isn't any ambiguity to which device we're waiting for. 

If --device isn't supplied, we could always verify that 'adb devices' only returns 1 device, and only swallow the exception in that case. Thoughts?
Comment on attachment 691927 [details] [diff] [review]
part 2: b2g update test frontend - v1

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

Pretty slick ;)

::: testing/marionette/client/marionette/b2g_update_test.py
@@ +83,5 @@
> +
> +        # The TestCase classes are apparently being loaded multiple times, so
> +        # using "isinstance" doesn't actually work. This function just compares
> +        # type names as a close enough analog.
> +        def has_super(cls, super_names):

Nice, I need to move this to CommonTestCase. :)

@@ +133,5 @@
> +    def port_forward(self, port):
> +        try:
> +            self.adb.run('forward', 'tcp:%d' % port, 'tcp:2828')
> +        except:
> +            # This command causes non-0 return codes even though it succeeds

I haven't seen this, it always returns an appropriate error code for me.  At any rate, using naked except clauses is usually not ideal, since it can hide other problems than the one you're trying to ignore.  Can we raise a more specific exception in adb.run?

@@ +196,5 @@
> +        try:
> +            self.marionette.execute_async_script(data, script_args=[self.testvars])
> +        except MarionetteException, e:
> +            # If the update test causes a restart, we will get a socket
> +            # exception here. TODO: do we need to make this more specific?

Yes, I think so; we should probably raise a very specific exception in that case, although that might have some downstream effects in other code already in production.  Feel free to file a bug for it.  Using MarionetteException here will e.g., hide JS errors, which isn't ideal.
Attachment #691927 - Flags: feedback?(jgriffin) → feedback+
Attachment #691929 - Flags: feedback?(jgriffin) → feedback+
(In reply to Marshall Culpepper [:marshall_law] from comment #7)
> (In reply to Jonathan Griffin (:jgriffin) from comment #6)
> > @@ +113,5 @@
> > > +
> > > +        try:
> > > +            adb.run('wait-for-device')
> > > +        except:
> > > +            # swallow the harmless "more than one device / emulator" error
> > 
> > It's not exactly harmless, since if that error occurs, it will mean we don't
> > end up waiting for the device.  Does the 'more than one device' error occur
> > during normal flashing, when you only have one device hooked up?  I haven't
> > seen it locally.
> 
> This may be a bootup oddity, but I see the error regularly with only 1
> device after "adb wait-for-device" returns from a freshly booted unagi. From
> my terminal, it looks like this:
> 
> % adb wait-for-device; print error_code=$?; adb devices
> [plug in a booting unagi, wait a while...]
> error: more than one device and emulator
> error_code=1
> List of devices attached 
> full_unagi	device
> 
> Probably the right way to fix this is to pass on the --device serial if it
> is supplied to Marionette, that way there isn't any ambiguity to which
> device we're waiting for. 
> 
> If --device isn't supplied, we could always verify that 'adb devices' only
> returns 1 device, and only swallow the exception in that case. Thoughts?

That's probably a reasonable workaround, if we combine that with a loop so we keep retrying until it succeeds.  Otherwise, this might return too soon and the subsequent adb forward command called elsewhere might fail.
Depends on: 821741
blocking-basecamp: ? → +
Adding in some Web QA peeps.
Comment on attachment 691959 [details] [diff] [review]
part 4: update_tools and test.sh support - v1

I don't think I know the testing stuff well enough to give feedback.
Attachment #691959 - Flags: feedback?(mwu)
Are we closer to landing this, Marshall?
Flags: needinfo?(marshall)
(In reply to Andrew Overholt [:overholt] from comment #12)
> Are we closer to landing this, Marshall?

I spoke with Marshall and he's polishing a few more bits before uploading a patch for review.  Speaking with jgriffin as well we hope to have this landed and running in the Jenkins system by next Friday (28th).
Flags: needinfo?(marshall)
Attachment #691926 - Attachment is obsolete: true
Attachment #695014 - Flags: review?(jgriffin)
Attachment #691927 - Attachment is obsolete: true
Attachment #695015 - Flags: review?(jgriffin)
Attachment #691929 - Attachment is obsolete: true
Attachment #695016 - Flags: review?(netzen)
This is a frontend aimed at CI for driving smoketests. Initial documentation is
here: https://wiki.mozilla.org/B2G/UpdateTesting
Attachment #695031 - Flags: review?(jgriffin)
Attachment #691959 - Attachment is obsolete: true
Attachment #695032 - Flags: review?(dhylands)
Comment on attachment 695014 [details] [diff] [review]
part 1: marionette changes - v2


> class NoSuchElementException(MarionetteException):
>     pass
>@@ -77,16 +80,17 @@ class ErrorCodes(object):
>     STALE_ELEMENT_REFERENCE = 10
>     ELEMENT_NOT_VISIBLE = 11
>     INVALID_ELEMENT_STATE = 12
>     UNKNOWN_ERROR = 13
>     ELEMENT_IS_NOT_SELECTABLE = 15
>     JAVASCRIPT_ERROR = 17
>     XPATH_LOOKUP_ERROR = 19
>     TIMEOUT = 21
>+    INVALID_RESPONSE = 22
>     NO_SUCH_WINDOW = 23

Can we change the Error code to be over 34 since all figures before 34 are reserved and would make us not compatible with the WebDriver Spec[1]

[1] http://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html
Attachment #695016 - Flags: review?(netzen) → review+
Comment on attachment 695032 [details] [diff] [review]
part 5: update_tools and test.sh support - v2

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

::: scripts/updates.sh
@@ +5,5 @@
> +. $B2G_DIR/setup.sh
> +
> +# Use default Gecko location if it's not provided in .config.
> +if [ -z $GECKO_PATH ]; then
> +  GECKO_PATH=$B2G_DIR/gecko

Not really a complaint or anything, just a tip for future reference.

You could also code this using:

GECKO_PATH="${GECKO_PATH:-${B2G_DIR}/gecko}"

::: tools/update-tools/build-flash-fota.py
@@ +20,5 @@
> +import os
> +import sys
> +import tempfile
> +import update_tools
> +

Justin had the following bit of boilerplate which would probably good to add:

if sys.version_info < (2,7):
    # We need Python 2.7 because we import argparse.
    print('This script requires Python 2.7.')
    sys.exit(1)
Attachment #695032 - Flags: review?(dhylands) → review+
Comment on attachment 695014 [details] [diff] [review]
part 1: marionette changes - v2

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

r+ with :AutomatedTester's note about moving the new error code to something > 34.

I'm also pushing to try: https://tbpl.mozilla.org/?tree=Try&rev=a53780802932, since this touches stuff already running in buildbot that we don't want to break.
Attachment #695014 - Flags: review?(jgriffin) → review+
Attachment #695031 - Flags: review?(jgriffin) → review+
Comment on attachment 695015 [details] [diff] [review]
part 2: b2g update test frontend - v2

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

I'd prefer to move this out of marionette/client and make it a real Python package that depends on Marionette.  But I'm ok with landing this as-is; I've filed bug 825284 to deal with the restructuring later.
Attachment #695015 - Flags: review?(jgriffin) → review+
> Justin had the following bit of boilerplate which would probably good to add:
> 
> if sys.version_info < (2,7):
>     # We need Python 2.7 because we import argparse.
>     print('This script requires Python 2.7.')
>     sys.exit(1)

I am doing this in my call to update_tools.validate_env() :)
Marshall, can you uplift this to b2g18?  That's where we'll be running the tests from.
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.