715215 - install robocop.apk for native android

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

in order to run robocop on tbpl, we need to install robocop.apk from the tests.zip:/bin folder.  This only exists on native fennec builds, not android-xul.  I have verified using sut_tools/installApp.py that we can install robocop.apk just fine.

In addition, we need to create a new test type: mochitest-robocop, which is run with a commandline similar to this:
 python runtestsremote.py --app=org.mozilla.fennec --deviceIP 1.2.3.4 --robocop=../bin --xre-path=/home/jmaher/mozilla/inbound/obj-i686-pc-linux-gnu/dist/bin

Comment 1

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

found in triage. bear said hwine had volunteered for this yesterday.

Comment 2

[Hal Wine [:hwine] (use NI)]

Attachment: make installApp.py testable

Needed to refactor this, to allow for multiple installs. Step one was to refactor for testability.

This diff should make no functional changes, but no tests to prove that, so wanted some eyeballs on it.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

Comment on attachment 586483 [details] [diff] [review]
make installApp.py testable

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

hmm, this just shows adding a main() function to installApp.py, overall a good idea and I don't see anything wrong with it.

Comment 4

[Hal Wine [:hwine] (use NI)]

status update: coding done & unit tested, working through first time test environment setup.

Still on track for delivery this week, as originally promised.

Comment 5

[Hal Wine [:hwine] (use NI)]

Test environment setup complete. Starting code change testing.

Comment 6

[Hal Wine [:hwine] (use NI)]

installing robocop.apk now works on tegras connected to foopy05. The cleanup can't be verified until after robocop is actually run.

Log file snippet:
Installing /mnt/sdcard/tests/robocop.apk
in push file with: build/robocop.apk, and: /mnt/sdcard/tests/robocop.apk
local hash returned: 'c756e818bc650687fba008a457a6ce1b'
sending: push /mnt/sdcard/tests/robocop.apk
push returned: c756e818bc650687fba008a457a6ce1b
$>
local hash returned: 'c756e818bc650687fba008a457a6ce1b'
Push File Validated!
------------------------------------------
installApp() done - gathering debug info

Comment 7

[Hal Wine [:hwine] (use NI)]

correction - the cleanup code is run every time, and is working.

Attaching code for review while Joel finishes.

Comment 8

[Hal Wine [:hwine] (use NI)]

Attachment: changes to installApp & cleanup

refactor to make testable, then addition of install of robocop.apk

Comment 9

[Hal Wine [:hwine] (use NI)]

Attachment: now that we have tests, run them

run nosetests on the sut_tools directory

Comment 10

[Mike Taylor [:bear]]

Comment on attachment 588541 [details] [diff] [review]
changes to installApp & cleanup

love most of the refactoring, except for the issues below:

- everything that is listed under the "first run" flag should just be in the __main__ code, this also removes the need for a global
- the calls to get|setDeviceTimestamp should be in the __main__ section - they don't need to be run for each install
- the time.sleep(60) needs to happen at the very end of the script
-in find_robocop() the exceptions should use the helper routine setFlag() so that clientproxy can know that the installApp() step error'd out

This may be a "flavour" thing, but most of what setup_communications() is doing are setting of core variables so I don't see the need to hide them out of the way - it reads better IMO to have them in __main__

Comment 11

[Hal Wine [:hwine] (use NI)]

Attachment: test cases for installApp

The tests - limited in coverage. Only basic characterization tests for the dm calls (both old & new)

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 588544 [details]
test cases for installApp

>#!/usr/bin/env python
>
>'''
>    As is tests for installApp.py prior to refactoring,
>
>    just going for characterization, not completeness
>'''
>
>import unittest
>import os
>from mock import Mock
>
># fake the DM, since it doesn't exist in repo land
># taken from http://code.activestate.com/recipes/82234-importing-a-dynamically-generated-module/
>import sys, imp
>def make_fake_module(name):
>    module = imp.new_module(name)
>    exec '' in module.__dict__
>    sys.modules[name] = module
>    return module
>
># now add the mocks we need to the dm
>devicemanagerSUT = make_fake_module('devicemanagerSUT')
>
># the_mock is the instance of DeviceManagerSUT class that will be
># instantiated. By setting it as the return value, no other mocking is
># done in the DeviceManagerSUT class.
>the_mock = Mock()
>devicemanagerSUT.DeviceManagerSUT = Mock(return_value=the_mock)
>
># since sut_lib methods are imported directly, we'll create the fake
># module, then add mocks for the cited routines
>sut_lib = make_fake_module('sut_lib')
>sut_lib = Mock()
>
># And the mocks we need from sut_lib
>import sut_lib
>def reset_mocks():
>    # we want to ensure a "success" from the installApp method
>    the_mock.installApp.return_value = None
>
>    # the routines we import by name from sut_lib, each need their own
>    # mock, so they are present at import time
>    sut_lib.getOurIP = Mock()
>    sut_lib.calculatePort = Mock()
>    sut_lib.clearFlag = Mock()
>    sut_lib.setFlag = Mock()
>    sut_lib.checkDeviceRoot = Mock(return_value='/fake_root')
>    sut_lib.getDeviceTimestamp = Mock()
>    sut_lib.setDeviceTimestamp = Mock()
>    sut_lib.getResolution = Mock(return_value=[1024, 768])
>    sut_lib.waitForDevice = Mock()
>    sut_lib.runCommand = Mock()
>    # don't exercise find code by default
>    os.path.exists = Mock(return_value=True)
>
>reset_mocks()
># we don't really want to wait during testing
>import time
>time.sleep = Mock()

This setup looks OK to me, but I'd suggest that they should go in the setUp method of a base class, and that reset_mocks() go in the tearDown method. Depending on the test runner you use, your tests aren't guaranteed to run in any particular order - so you can't, for example, assume CheckBasicExecution tests will run before the other ones.

>class CheckBasicExecution(unittest.TestCase):
>    def test_error_short_args(self):
>        try:
>            installApp.main(['app',1])
>        except SystemExit as e:
>            self.assertEqual(1, e.code)
>        else:
>            self.fail('should have thrown SystemExit')

This can simplified to:
self.assertRaises(SystemExit, installApp.main, ['app, 1])


>class CheckOriginalContract(unittest.TestCase):
>    def setUp(self):
>        the_mock.reset_mock()
>
>    # bummer expectedFailure decorator is not supported before py 2.7
>    # unittest, so just disable the test
>    # @unittest.expectedFailure

If this is going to fail every time, it needs to be commented out for now.


Looks fine overall, I'd like to see the structure changed and the tests raise exceptions simplified.

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 588543 [details] [diff] [review]
now that we have tests, run them

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

Sorry - I don't feel qualified to review this code, I never worked on it.

Comment 14

[Hal Wine [:hwine] (use NI)]

Attachment: test cases for installApp

Incorporated prior feedback

Comment 15

[[e-mail] (:bhearsum)]

Comment on attachment 589614 [details]
test cases for installApp

>#!/usr/bin/env python
>
>'''
>    As is tests for installApp.py prior to refactoring,
>
>    just going for characterization, not completeness
>'''
>
>import unittest
>import os
>from mock import Mock
>
># fake the DM, since it doesn't exist in repo land
># taken from http://code.activestate.com/recipes/82234-importing-a-dynamically-generated-module/
>import sys, imp
>def make_fake_module(name):
>    module = imp.new_module(name)
>    exec '' in module.__dict__
>    sys.modules[name] = module
>    return module
>
># now add the mocks we need to the dm
>devicemanagerSUT = make_fake_module('devicemanagerSUT')
>
># the_mock is the instance of DeviceManagerSUT class that will be
># instantiated. By setting it as the return value, no other mocking is
># done in the DeviceManagerSUT class.
>the_mock = Mock()
>devicemanagerSUT.DeviceManagerSUT = Mock(return_value=the_mock)

Right now the test that depends on this module works, but you'll get non-deterministic behaviour if you could get confusing test failures if you end up with multiple tests sharing this same object in the future. For this reason, I think that devicemanagerSUT, the_mock, and sut_lib should be created in setUp of InstallAppTestCase as member data, instead of a module-level object. Doing so will ensure that ever test gets a clean state before it runs. Looks fine otherwise, though.

Comment 16

[Hal Wine [:hwine] (use NI)]

Attachment: changes to installApp & cleanup

changes per review comments

Comment 17

[Mike Taylor [:bear]]

Comment on attachment 589695 [details] [diff] [review]
changes to installApp & cleanup

isn't this the same patch as above?

Comment 18

[[e-mail] (:bhearsum)]

Comment on attachment 589614 [details]
test cases for installApp

Spoke with Hal on the phone about this. He told me that this code isn't expected to be long lived. Based on that, I'm OK with this.

Comment 19

[Hal Wine [:hwine] (use NI)]

Comment on attachment 588543 [details] [diff] [review]
now that we have tests, run them

this is not the code to run the tests - wrong content, and this code revised in later attachment

Comment 20

[Hal Wine [:hwine] (use NI)]

Attachment: don't fail if robocop.apk can't be found

Changes to build robocop.apk are only on m-c & m-a currently, but foopy changes affect all branches. Change the code to not attempt install if robocop.apk can't be found (with messages to the log file).

If robocopy is required and not present, the subsequent test will need to fail.

Comment 21

[Mike Taylor [:bear]]

Comment on attachment 590803 [details] [diff] [review]
don't fail if robocop.apk can't be found

s/not as exected/not as expected/

r+ with that change

Comment 22

[Hal Wine [:hwine] (use NI)]

update summary to reflect new test type now covered by bug 719443

Comment 23

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

is this done?  I see all patches are checked in?  Are they live?

Comment 24

[Hal Wine [:hwine] (use NI)]

This code is now live on all foopies (as of noon PT Jan 24). Was planning on waiting to close it until we have end-to-end verification, but we can reopen if needed.

Comment 25

[Hal Wine [:hwine] (use NI)]

Final changes to bug 719697 requires changes to the way robocop.apk installed.

Comment 26

[Hal Wine [:hwine] (use NI)]

Attachment: download robocop.apk to foopy

2 changes: first to download robocop.apk, second to pick up robocop.ini file from new location.

Comment 27

[Aki Sasaki (not active)]

Comment on attachment 592601 [details] [diff] [review]
download robocop.apk to foopy

>+            print "built robocop url '%s' from '%s'" % (robocop_url, build_url)

Remove the print when you're done testing.

>+        self.addStep(DownloadFile(
>+            url_fn=get_robocop_url,
>+            filename_property='robocop_filename',
>+            url_property='robocop_url',
>+            haltOnFailure=False,

Glad that we don't halt on failure, but aiui this will flunkOnFailure, which means whenever it can't download robocop.apk it'll fail.
This patch doesn't limit downloading robocop.apk to any test or platform, so all android-xul and linux-android runs will go red when this lands.
Minusing for that.

I think you need to conditionally download.

Also, you seem to be picking up a bad habit from Bear, which is asking for review before a patch is tested.
I think that's fine when there's high confidence that a patch will be good without testing; otherwise it's best to set up a staging env on dev-master01 and verify this thing works first.

Comment 28

[Hal Wine [:hwine] (use NI)]

(In reply to Aki Sasaki [:aki] from comment #27)
> Comment on attachment 592601 [details] [diff] [review]
> download robocop.apk to foopy
> 
> >+            print "built robocop url '%s' from '%s'" % (robocop_url, build_url)
> 
> Remove the print when you're done testing.

I'll pull the print, as all positive test cases already pass, of course, in my single-tegra staging environment.

> 
> >+        self.addStep(DownloadFile(
> >+            haltOnFailure=False,
> 
> Glad that we don't halt on failure, but aiui this will flunkOnFailure, which
> means whenever it can't download robocop.apk it'll fail.

Good catch - saves a pass of negative test cases in staging.

> This patch doesn't limit downloading robocop.apk to any test or platform, so
> all android-xul and linux-android runs will go red when this lands.
> Minusing for that.

Agreed that it can't land if it causes collateral damage. Setting the appropriate flags as you requested above will address that. 

> 
> I think you need to conditionally download.

The current approach (non-conditional) was an active design choice at the start of this process, and reflects the nature of where things are w.r.t. robocop testing.

The goal is to always download and install robocop.apk if it exists in the build.

Comment 29

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

My understanding is we are done with this and waiting for everything else to be resolved before closing this bug?

Comment 30

[Hal Wine [:hwine] (use NI)]

(In reply to Joel Maher (:jmaher) from comment #29)
> My understanding is we are done with this and waiting for everything else to
> be resolved before closing this bug?

Correct.

Comment 31

[Hal Wine [:hwine] (use NI)]

Attachment: download robocop.apk to foopy

tests to date: 
 - creates clean config
 - does the right thing in my dev environment

testing underway:
 - "burn in" in dev staging

Comment 32

[Aki Sasaki (not active)]

Comment on attachment 594019 [details] [diff] [review]
download robocop.apk to foopy

>+        # the goal of bug 715215 is to download robocop.apk if it
>+        # exists. If it doesn't exist, be quiet. It's up to the tests to
>+        # complain. This is because robocop is needed for some, but
>+        # not all android tests.
>+        if self.platform == "android":
>+            self.addStep(DownloadFile(
>+                url_fn=get_robocop_url,
>+                filename_property='robocop_filename',
>+                url_property='robocop_url',
>+                haltOnFailure=False,
>+                flunkOnWarnings=False,
>+                flunkOnFailure=False,
>+                warnOnWarnings=True,
>+                warnOnFailure=True,
>+                ignore_certs=self.ignoreCerts,
>+                name='download_robocop',
>+            ))

I think this will work.
Does it turn jobs orange when they fail to download it, though?
That may affect test results.

Let me know in staging -- we may want to change the warnOn{Warnings,Failure} to False.

Otherwise this looks good.

Comment 33

[Hal Wine [:hwine] (use NI)]

Comment on attachment 594019 [details] [diff] [review]
download robocop.apk to foopy

Verbal r+ from aki on minor tweaks to posted patch. Landed at

http://hg.mozilla.org/build/buildbotcustom/rev/5a2cb72a051a

Comment 34

[Armen [:armenzg]]

Is this done?

Comment 35

[Hal Wine [:hwine] (use NI)]

reconfig deployed this today at 1300 PST. So done. (yay!)