Closed
Bug 605176
Opened 14 years ago
Closed 13 years ago
change factories and buildbot-config to support unittests on Android
Categories
(Release Engineering :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bear, Assigned: bhearsum)
References
Details
(Whiteboard: [android])
Attachments
(8 files, 6 obsolete files)
15.02 KB,
patch
|
mozilla
:
feedback+
|
Details | Diff | Splinter Review |
29.23 KB,
patch
|
mozilla
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
9.50 KB,
patch
|
mozilla
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
1.42 KB,
patch
|
mozilla
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
6.00 KB,
patch
|
mozilla
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
1.32 KB,
patch
|
mozilla
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
467 bytes,
patch
|
mozilla
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
1.15 KB,
patch
|
mozilla
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
Fix/add changes to enable reftest on Tegra boards using SUTAgentAndroid
Reporter | ||
Updated•14 years ago
|
Whiteboard: [android]
Reporter | ||
Comment 1•14 years ago
|
||
Attachment #487927 -
Flags: feedback?(aki)
Comment 2•14 years ago
|
||
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+
Comment 3•13 years ago
|
||
Bear: is this wip patch your latest, or do you have more?
Reporter | ||
Comment 4•13 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
Comment 5•13 years ago
|
||
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•13 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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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 11•13 years ago
|
||
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 12•13 years ago
|
||
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•13 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.
Comment 14•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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)
Comment 17•13 years ago
|
||
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•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #514932 -
Flags: review?(aki) → review+
Updated•13 years ago
|
Attachment #515072 -
Flags: review?(aki) → review+
Assignee | ||
Comment 19•13 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•13 years ago
|
Attachment #514932 -
Flags: checked-in+
Assignee | ||
Comment 20•13 years ago
|
||
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•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #515753 -
Flags: review?(aki) → review+
Updated•13 years ago
|
Attachment #515754 -
Flags: review?(aki) → review+
Assignee | ||
Comment 22•13 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•13 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•13 years ago
|
||
Per latest from #ateam, 2 chunks is going to be better than 4.
Attachment #515921 -
Flags: review?(aki)
Updated•13 years ago
|
Attachment #515921 -
Flags: review?(aki) → review+
Assignee | ||
Comment 25•13 years ago
|
||
Comment on attachment 515921 [details] [diff] [review] use 2 reftest chunks Landed this on default, too.
Assignee | ||
Comment 26•13 years ago
|
||
Until bug 637660 is fixed we're getting to lots of timeouts in crashtest, and tie up Tegras unnecessarily.
Attachment #516267 -
Flags: review?(aki)
Updated•13 years ago
|
Attachment #516267 -
Flags: review?(aki) → review+
Assignee | ||
Comment 27•13 years ago
|
||
Comment on attachment 516267 [details] [diff] [review] disable crashtest Landed on default
Assignee | ||
Comment 28•13 years ago
|
||
Turns out we need to set this for the summaries to be useful.
Attachment #516345 -
Flags: review?(aki)
Assignee | ||
Comment 29•13 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)
Updated•13 years ago
|
Attachment #516374 -
Flags: review?(aki) → review+
Assignee | ||
Comment 31•13 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•13 years ago
|
Attachment #515754 -
Flags: checked-in+
Assignee | ||
Updated•13 years ago
|
Attachment #515753 -
Flags: checked-in+
Assignee | ||
Updated•13 years ago
|
Attachment #515921 -
Flags: checked-in+
Assignee | ||
Updated•13 years ago
|
Attachment #516267 -
Flags: checked-in+
Assignee | ||
Comment 32•13 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
Closed: 13 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•