change factories and buildbot-config to support unittests on Android

RESOLVED FIXED

Status

Release Engineering
General
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: bear, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [android])

Attachments

(8 attachments, 6 obsolete attachments)

15.02 KB, patch
Details | Diff | Splinter Review
29.23 KB, patch
aki
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
9.50 KB, patch
aki
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
1.42 KB, patch
aki
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
6.00 KB, patch
aki
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
1.32 KB, patch
aki
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
467 bytes, patch
aki
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
1.15 KB, patch
aki
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
Fix/add changes to enable reftest on Tegra boards using SUTAgentAndroid
(Reporter)

Updated

7 years ago
Whiteboard: [android]
(Reporter)

Updated

7 years ago
Assignee: nobody → bear
Blocks: 561908
(Reporter)

Updated

7 years ago
Depends on: 605207
(Reporter)

Updated

7 years ago
Depends on: 572572
(Reporter)

Comment 1

7 years ago
Created attachment 487927 [details] [diff] [review]
[WIP] factory changes to enable remote reftests
Attachment #487927 - Flags: feedback?(aki)
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.
Attachment #487927 - Flags: feedback?(aki) → feedback+
Bear: is this wip patch your latest, or do you have more?
(Reporter)

Comment 4

7 years ago
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
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.
Assignee: bear → bhearsum
Summary: changes to factory and buildbot-config for Android RefTests → change factories and buildbot-config to support unittests on Android
(Reporter)

Comment 6

7 years ago
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
(Assignee)

Comment 7

7 years ago
Created attachment 513556 [details] [diff] [review]
[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.
(Assignee)

Comment 8

7 years ago
Created attachment 513557 [details] [diff] [review]
[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.
(Assignee)

Comment 9

7 years ago
Created attachment 514558 [details] [diff] [review]
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?
Attachment #513556 - Attachment is obsolete: true
Attachment #513557 - Attachment is obsolete: true
Attachment #514558 - Flags: feedback?(aki)
(Assignee)

Comment 10

7 years ago
Created attachment 514559 [details] [diff] [review]
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.
Attachment #514559 - Flags: feedback?(aki)
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 :)
Attachment #514558 - Flags: feedback?(aki) → feedback+
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 :)
Attachment #514559 - Flags: feedback?(aki) → feedback+
(Assignee)

Comment 13

7 years ago
(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.
(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.
(Assignee)

Comment 15

7 years ago
Created attachment 514932 [details] [diff] [review]
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
Attachment #514558 - Attachment is obsolete: true
Attachment #514559 - Attachment is obsolete: true
Attachment #514932 - Flags: review?(aki)
(Assignee)

Comment 16

7 years ago
Created attachment 514933 [details] [diff] [review]
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.
Attachment #514933 - Flags: review?(aki)
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.
(Assignee)

Comment 18

7 years ago
Created attachment 515072 [details] [diff] [review]
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.
Attachment #514933 - Attachment is obsolete: true
Attachment #515072 - Flags: review?(aki)
Attachment #514933 - Flags: review?(aki)
Attachment #514932 - Flags: review?(aki) → review+
Attachment #515072 - Flags: review?(aki) → review+
(Assignee)

Comment 19

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

Landed on default, planning to merge and reconfig a bit later.
Attachment #515072 - Flags: checked-in+
(Assignee)

Updated

7 years ago
Attachment #514932 - Flags: checked-in+
(Assignee)

Comment 20

7 years ago
Created attachment 515753 [details] [diff] [review]
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.
Attachment #515753 - Flags: review?(aki)
(Assignee)

Comment 21

7 years ago
Created attachment 515754 [details] [diff] [review]
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).
Attachment #515754 - Flags: review?(aki)
Attachment #515753 - Flags: review?(aki) → review+
Attachment #515754 - Flags: review?(aki) → review+
(Assignee)

Comment 22

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

Landed this on default:
changeset:   4087:86337c02f720
(Assignee)

Comment 23

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

changeset:   1562:d806855cf1dc
(Assignee)

Comment 24

7 years ago
Created attachment 515921 [details] [diff] [review]
use 2 reftest chunks

Per latest from #ateam, 2 chunks is going to be better than 4.
Attachment #515921 - Flags: review?(aki)
Attachment #515921 - Flags: review?(aki) → review+
(Assignee)

Comment 25

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

Landed this on default, too.
(Assignee)

Updated

7 years ago
Depends on: 637660
(Assignee)

Comment 26

7 years ago
Created attachment 516267 [details] [diff] [review]
disable crashtest

Until bug 637660 is fixed we're getting to lots of timeouts in crashtest, and tie up Tegras unnecessarily.
Attachment #516267 - Flags: review?(aki)
Attachment #516267 - Flags: review?(aki) → review+
(Assignee)

Comment 27

7 years ago
Comment on attachment 516267 [details] [diff] [review]
disable crashtest

Landed on default
(Assignee)

Comment 28

7 years ago
Created attachment 516345 [details] [diff] [review]
set 'name' in remote steps

Turns out we need to set this for the summaries to be useful.
Attachment #516345 - Flags: review?(aki)
(Assignee)

Comment 29

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

posted this too soon
Attachment #516345 - Attachment is obsolete: true
Attachment #516345 - Flags: review?(aki)
(Assignee)

Comment 30

7 years ago
Created attachment 516374 [details] [diff] [review]
set name for remote steps

Tested version.
Attachment #516374 - Flags: review?(aki)
Attachment #516374 - Flags: review?(aki) → review+
(Assignee)

Comment 31

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

Landed on default: changeset:   1565:3eb5db49c19f
Attachment #516374 - Flags: checked-in+
(Assignee)

Updated

7 years ago
No longer depends on: 572572
(Assignee)

Updated

7 years ago
Attachment #515754 - Flags: checked-in+
(Assignee)

Updated

7 years ago
Attachment #515753 - Flags: checked-in+
(Assignee)

Updated

7 years ago
Attachment #515921 - Flags: checked-in+
(Assignee)

Updated

7 years ago
Attachment #516267 - Flags: checked-in+
(Assignee)

Comment 32

7 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.