605176 - change factories and buildbot-config to support unittests on Android

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Mike Taylor [:bear]]

Fix/add changes to enable reftest on Tegra boards using SUTAgentAndroid

Comment 1

[Mike Taylor [:bear]]

Attachment: [WIP] factory changes to enable remote reftests

Comment 2

[Aki Sasaki (not active)]

Comment on attachment 487927 [details] [diff] [review]
[WIP] factory changes to enable remote reftests

So, as mentioned in IRC (with some amendments):

The main thing for this pass is, does it work as is?

Once that's good, we need to add remoteTests and remoteExtras type info in config.py for unit tests (I have not specifically looked at how to do this, let me know if you need help), and pass those in to the unit test factory in generateTalosBranchObjects if it's defined.  Then tear all the info that's hardcoded out, and put it in the dict.

One thing we need to be careful of, that happened in the Talos change is we borked something in desktop land (my fault, it was the tinderbox reporting).  So once we have a good patch that passes the eyeball test, a quick run in desktop staging might be in order.

Comment 3

[Aki Sasaki (not active)]

Bear: is this wip patch your latest, or do you have more?

Comment 4

[Mike Taylor [:bear]]

I would like to have more, but yes that is the latest.  what I was doing locally was checking the bit-rotted'ness of the patch and checking to see if my local copy was better

Comment 5

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

from offline discussions with aki, bear, bhearsum, coop:

1) aki thinks that reftest was the first suite being tried, because it was the only suite that worked at the time. However, this bug is really about being able to detect, and queue, all unittest suites which are dependent on an android build. Morphing summary to match.

2) bhearsum willing to help out and take this on, so reassigning (with thanks!).

3) Once the jobs are queued up, and being accepted by the foopys, its up to clientproxy to handle accepting the jobs and communicating back/forth with SUTagent. That separate work is being tracked in bug#579560.

Comment 6

[Mike Taylor [:bear]]

from an irc discussion with jmaher - these are the mochitest commands that he had given me to help sanity check:

python runtestsremote.py --deviceIP=192.168.1.102 --test-path=gfx --xre-path=../fennec --utility-path=../bin  --certificate-path=../certs/ --app=org.mozilla.fennec

python runtestsremote.py --deviceIP=192.168.1.102 --browser-chrome --xre-path=../fennec --utility-path=../bin  --certificate-path=../certs/ --app=org.mozilla.fennec 

with appropriate IP changes

Comment 7

[[e-mail] (:bhearsum)]

Attachment: [wip] create test factory, steps for mochitest-through-runtestsremote.py

This patch takes a slightly different approach than Bear's, and creates a new MozillaTestFactory subclass for remote tests, because they share almost nothing in common with their non-remote counterparts. Similar for steps/unittest.py -- I've factored out the common mochitest bits there, and created a new class for remote ones.

Been able to run various plain mochitests with --test-path as well as --browser-chrome.

Comment 8

[[e-mail] (:bhearsum)]

Attachment: [wip] config adjustments

This adjustment to the configs sets up one Builder per mochitest test dir, and one for browser-chrome tests. I'm running with this right now to try and figure out a good split for the directories.

Comment 9

[[e-mail] (:bhearsum)]

Attachment: buildbotcustom patch, mostly complete

This patch has been pretty well tested on the mobile side, and is capable of running mochitests and browser chrome. I haven't completed testing on the reftest bits yet, but the current configs don't enable them. Summary:
- Factor out Mochitest/Reftest log parsing and common args to Mixins/functions
- Create BuildStep subclasses for remotereftest.py and runtestsremote.py
- Create RemoteUnittestFactory, based on MozillaTestFactory
- Add unittestMasters support to MobileBuildFactory
- Glue in misc.py

I still need to verify that these patches don't bust desktop unittests in any way, but I think they're ready for feedback?

Comment 10

[[e-mail] (:bhearsum)]

Attachment: configs patch

Fairly simple patch, summary:
- Alter SLAVES' values to be a dict, whose keys are the slave names and whose values are slave properties, and set http_port and ssl_port for Tegras.
- Set opt_unittest_suites for android.

Comment 11

[Aki Sasaki (not active)]

Comment on attachment 514558 [details] [diff] [review]
buildbotcustom patch, mostly complete

>+    # HACKHACKHACKHACKHACKHACK
>+    if 'android' in platform:

As mentioned in IRC, I asked Bear to change all of his 'android' in platform checks to check a remoteTests flag.
Hoping that's easy to put into PLATFORM_UNITTEST_VARS or something.

>+    def addTearDownSteps(self):
>+        self.addCleanupSteps()
>+        def do_disconnect(cmd):
>+            try:
>+                if 'SCHEDULED REBOOT' in cmd.logs['stdio'].getText():
>+                    return True
>+            except:
>+                pass
>+            return False
>+        self.addStep(DisconnectStep(
>+         name='reboot device',
>+         alwaysRun=True,
>+         workdir='.',
>+         description="Reboot Device",
>+         command=['python', '../../sut_tools/reboot.py',
>+                  WithProperties("%(sut_ip)s"),
>+                 ],
>+         force_disconnect=do_disconnect,
>+        ))

I don't think we currently disconnect during the reboot step at all, or get SCHEDULED REBOOT from reboot.py, but it's good to have this available if/when we switch to that.

>+def evaluateMochitest(name, log, superResult):

I think it's good pulling these out and creating the Mixins so we have less duplication of code.
It does mean that there's a higher chance for breaking desktop tests, so yes, testing those is good :)

Comment 12

[Aki Sasaki (not active)]

Comment on attachment 514559 [details] [diff] [review]
configs patch

>+    'tegra_android': dict([('tegra-%03i' % x, {'http_port': '30%03i' % x, 'ssl_port': '31%03i' % x}) for x in range(1,95)]),

Very clever :)

Comment 13

[[e-mail] (:bhearsum)]

(In reply to comment #11)
> >+    def addTearDownSteps(self):
> >+        self.addCleanupSteps()
> >+        def do_disconnect(cmd):
> >+            try:
> >+                if 'SCHEDULED REBOOT' in cmd.logs['stdio'].getText():
> >+                    return True
> >+            except:
> >+                pass
> >+            return False
> >+        self.addStep(DisconnectStep(
> >+         name='reboot device',
> >+         alwaysRun=True,
> >+         workdir='.',
> >+         description="Reboot Device",
> >+         command=['python', '../../sut_tools/reboot.py',
> >+                  WithProperties("%(sut_ip)s"),
> >+                 ],
> >+         force_disconnect=do_disconnect,
> >+        ))
> 
> I don't think we currently disconnect during the reboot step at all, or get
> SCHEDULED REBOOT from reboot.py, but it's good to have this available if/when
> we switch to that.

I actually copy/pasted this without thought, and now that you point this out it makes little sense. I'm going to switch this to a plain ShellCommand. Since these are remotes, we don't have to deal with the buildslave-disconnecting-at-the-end issue -- no need to complicate this part.

> >+def evaluateMochitest(name, log, superResult):
> 
> I think it's good pulling these out and creating the Mixins so we have less
> duplication of code.
> It does mean that there's a higher chance for breaking desktop tests, so yes,
> testing those is good :)

Thanks! I almost moved these into build/tools/lib/python, but it was too much hassle for now. Should be simple to do it in the future, though.

Comment 14

[Aki Sasaki (not active)]

(In reply to comment #13)
> > I don't think we currently disconnect during the reboot step at all, or get
> > SCHEDULED REBOOT from reboot.py, but it's good to have this available if/when
> > we switch to that.
> 
> I actually copy/pasted this without thought, and now that you point this out it
> makes little sense. I'm going to switch this to a plain ShellCommand. Since
> these are remotes, we don't have to deal with the
> buildslave-disconnecting-at-the-end issue -- no need to complicate this part.

Somewhere down the road we may shut down the build slave when the reboot happens, and restart it when it reconnects.  That's not a given, but leaving it won't hurt.

Cleaning it up and changing again if/when we do shut down the build slave is ok as well.

Comment 15

[[e-mail] (:bhearsum)]

Attachment: buildbotcustm patch, updated

Changes in this patch:
- Switch to is_remote
- Cope with slightly altered suites data structure
- Run tests out of "tests" instead of "mochitest", to match Desktop unittests
- Fix some bugs w.r.t. reftest in unittest.py

Comment 16

[[e-mail] (:bhearsum)]

Attachment: update configs patch

Changes in this version:
- Download host utils from bm-remote.b.m.o
- Change opt_unittest_suites structure a bit, so we don't have to pass "testPaths" to the Factory (because it only applies to Mochitest, not all test types)
- Drop unused debug_unittest_suites arg

I've tested all the desktop unittests and fixed a few issues. I've tested both the builder-side changes (sendchange additions) as well as the test-side changes (all the mochitest builders). Both work fine AFAICT.

I'm close to being able to enable reftest, too, but I wanted to get this up in the meantime.

Comment 17

[Aki Sasaki (not active)]

I wanted to get to these before I finished for the day, but I will have to postpone til tomorrow, where tomorrow is still Friday.

Comment 18

[[e-mail] (:bhearsum)]

Attachment: updated configs, with reftest

I got reftests running this morning. They'll fail in production until https://bugzilla.mozilla.org/show_bug.cgi?id=636641 lands, but I think that's OK. Joel says that this should be landing sometime today.

Comment 19

[[e-mail] (:bhearsum)]

Comment on attachment 515072 [details] [diff] [review]
updated configs, with reftest

Landed on default, planning to merge and reconfig a bit later.

Comment 20

[[e-mail] (:bhearsum)]

Attachment: split reftest into 4, enable crashtest

Per jmaher's request, this patch splits reftest into 4 builders. I've also enabled crashtest which seems to be running to completion now.

Comment 21

[[e-mail] (:bhearsum)]

Attachment: Fix for..else logic error, add chunking support/http-port for reftest

A fix fixes here:
- Fix my logic error with "for...else" -- turns out that you have to "break" in order to have the "else" condition skipped over. I've changed this to simpler if/else logic.
- Adds chunking support for reftest in RemoteUnittestFactory. I factored out the chunking options into another mixin class.
- Adds http_port/ssl_port support for the RemoteReftestStep -- I forgot to do this in my first patches, which meant all reftests would listen on port 8888.

I've tested this in tegra staging as well as limitedly in desktop test machine staging (to ensure the chunking changes didn't break anything).

Comment 22

[[e-mail] (:bhearsum)]

Comment on attachment 515753 [details] [diff] [review]
split reftest into 4, enable crashtest

Landed this on default:
changeset:   4087:86337c02f720

Comment 23

[[e-mail] (:bhearsum)]

Comment on attachment 515754 [details] [diff] [review]
Fix for..else logic error, add chunking support/http-port for reftest

changeset:   1562:d806855cf1dc

Comment 24

[[e-mail] (:bhearsum)]

Attachment: use 2 reftest chunks

Per latest from #ateam, 2 chunks is going to be better than 4.

Comment 25

[[e-mail] (:bhearsum)]

Comment on attachment 515921 [details] [diff] [review]
use 2 reftest chunks

Landed this on default, too.

Comment 26

[[e-mail] (:bhearsum)]

Attachment: disable crashtest

Until bug 637660 is fixed we're getting to lots of timeouts in crashtest, and tie up Tegras unnecessarily.

Comment 27

[[e-mail] (:bhearsum)]

Comment on attachment 516267 [details] [diff] [review]
disable crashtest

Landed on default

Comment 28

[[e-mail] (:bhearsum)]

Attachment: set 'name' in remote steps

Turns out we need to set this for the summaries to be useful.

Comment 29

[[e-mail] (:bhearsum)]

Comment on attachment 516345 [details] [diff] [review]
set 'name' in remote steps

posted this too soon

Comment 30

[[e-mail] (:bhearsum)]

Attachment: set name for remote steps

Tested version.

Comment 31

[[e-mail] (:bhearsum)]

Comment on attachment 516374 [details] [diff] [review]
set name for remote steps

Landed on default: changeset:   1565:3eb5db49c19f

Comment 32

[[e-mail] (:bhearsum)]

The work that this bug describes is done. We still need to enable crashtest (tracked in 638844), and jsreftest (tracked in bug 638846). There may also be tweaks to reftest, which will go in a new bug.