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)
Tracking
(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)
People
(Reporter: marshall, Assigned: marshall)
References
Details
Attachments
(5 files, 4 obsolete files)
17.36 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
24.34 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
9.25 KB,
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
13.83 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
38.67 KB,
patch
|
dhylands
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Marionette changes to support the new update test frontend
Attachment #691926 -
Flags: review?(jgriffin)
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #691926 -
Flags: review?(jgriffin) → feedback?(jgriffin)
Assignee | ||
Updated•11 years ago
|
Attachment #691927 -
Flags: feedback?(jgriffin)
Assignee | ||
Updated•11 years ago
|
Attachment #691929 -
Flags: feedback?(netzen)
Attachment #691929 -
Flags: feedback?(jones.chris.g)
Attachment #691929 -
Flags: feedback?(jgriffin)
Updated•11 years ago
|
Attachment #691929 -
Flags: feedback?(jones.chris.g) → feedback+
Assignee | ||
Comment 4•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #691959 -
Flags: feedback?(mwu)
Attachment #691959 -
Flags: feedback?(dhylands)
Assignee | ||
Updated•11 years ago
|
blocking-basecamp: --- → ?
Updated•11 years ago
|
Attachment #691929 -
Flags: feedback?(netzen) → feedback+
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
(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 8•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #691929 -
Flags: feedback?(jgriffin) → feedback+
Comment 9•11 years ago
|
||
(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.
Updated•11 years ago
|
blocking-basecamp: ? → +
Adding in some Web QA peeps.
Comment 11•11 years ago
|
||
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)
Comment 13•11 years ago
|
||
(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)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #691926 -
Attachment is obsolete: true
Attachment #695014 -
Flags: review?(jgriffin)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #691927 -
Attachment is obsolete: true
Attachment #695015 -
Flags: review?(jgriffin)
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #691929 -
Attachment is obsolete: true
Attachment #695016 -
Flags: review?(netzen)
Assignee | ||
Comment 17•11 years ago
|
||
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)
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #691959 -
Attachment is obsolete: true
Attachment #695032 -
Flags: review?(dhylands)
Comment 19•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #695016 -
Flags: review?(netzen) → review+
Comment 20•11 years ago
|
||
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 21•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #695031 -
Flags: review?(jgriffin) → review+
Comment 22•11 years ago
|
||
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+
Assignee | ||
Comment 23•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d41cea7a8ce https://hg.mozilla.org/integration/mozilla-inbound/rev/ca03917bee50 https://hg.mozilla.org/integration/mozilla-inbound/rev/80ab5bc94810 https://hg.mozilla.org/integration/mozilla-inbound/rev/067e0c9943e3
Assignee | ||
Comment 24•11 years ago
|
||
> 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() :)
Comment 25•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4d41cea7a8ce https://hg.mozilla.org/mozilla-central/rev/ca03917bee50 https://hg.mozilla.org/mozilla-central/rev/80ab5bc94810 https://hg.mozilla.org/mozilla-central/rev/067e0c9943e3
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 26•11 years ago
|
||
Marshall, can you uplift this to b2g18? That's where we'll be running the tests from.
Comment 27•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b246aa8680e5 https://hg.mozilla.org/releases/mozilla-aurora/rev/1649930d502f https://hg.mozilla.org/releases/mozilla-aurora/rev/5f029e9fe8fd https://hg.mozilla.org/releases/mozilla-aurora/rev/e88a1b5f06ff https://hg.mozilla.org/releases/mozilla-b2g18/rev/59b8e3e5a1f3 https://hg.mozilla.org/releases/mozilla-b2g18/rev/d4f582256841 https://hg.mozilla.org/releases/mozilla-b2g18/rev/fa60c88b384c https://hg.mozilla.org/releases/mozilla-b2g18/rev/4864a3121cce
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•