install robocop.apk for native android

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jmaher, Assigned: hwine)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mobile_unittests][android_tier_1])

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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
found in triage. bear said hwine had volunteered for this yesterday.
Assignee: nobody → hwine
Created attachment 586483 [details] [diff] [review]
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.
Attachment #586483 - Flags: feedback?(jmaher)
Attachment #586483 - Flags: feedback?(bear)
(Reporter)

Comment 3

7 years ago
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.
Attachment #586483 - Flags: feedback?(jmaher) → feedback+

Updated

7 years ago
Attachment #586483 - Flags: feedback?(bear) → feedback+
status update: coding done & unit tested, working through first time test environment setup.

Still on track for delivery this week, as originally promised.
Test environment setup complete. Starting code change testing.
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
correction - the cleanup code is run every time, and is working.

Attaching code for review while Joel finishes.
Created attachment 588541 [details] [diff] [review]
changes to installApp & cleanup

refactor to make testable, then addition of install of robocop.apk
Attachment #588541 - Flags: review?(bear)
Attachment #588541 - Flags: review?(aki)
Created attachment 588543 [details] [diff] [review]
now that we have tests, run them

run nosetests on the sut_tools directory
Attachment #588543 - Flags: review?(bhearsum)
Attachment #588543 - Flags: review?(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__
Attachment #588541 - Flags: review?(bear) → review-
Created attachment 588544 [details]
test cases for installApp

The tests - limited in coverage. Only basic characterization tests for the dm calls (both old & new)
Attachment #588544 - Flags: review?(bhearsum)
Attachment #588544 - Flags: review?(bear)
Attachment #588543 - Attachment is patch: true
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.
Attachment #588544 - Flags: review?(bhearsum) → review-
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.
Attachment #588543 - Flags: review?(bhearsum)
Created attachment 589614 [details]
test cases for installApp

Incorporated prior feedback
Attachment #588544 - Attachment is obsolete: true
Attachment #588544 - Flags: review?(bear)
Attachment #589614 - Flags: review?(bhearsum)
(Assignee)

Updated

7 years ago
Attachment #588544 - Attachment is obsolete: false
(Assignee)

Updated

7 years ago
Attachment #588544 - Attachment is obsolete: true
Attachment #589614 - Attachment mime type: text/x-python-script → text/plain
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.
Attachment #589614 - Flags: review?(bhearsum) → review-
Created attachment 589695 [details] [diff] [review]
changes to installApp & cleanup

changes per review comments
Attachment #588541 - Attachment is obsolete: true
Attachment #588541 - Flags: review?(aki)
Attachment #589695 - Flags: review?(bear)

Updated

7 years ago
Attachment #588543 - Flags: review?(bear) → review+
Comment on attachment 589695 [details] [diff] [review]
changes to installApp & cleanup

isn't this the same patch as above?
Attachment #589695 - Flags: review?(bear)
(Assignee)

Updated

7 years ago
Attachment #589695 - Flags: review?(bear)

Updated

7 years ago
Attachment #589695 - Flags: review?(bear) → review+
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.
Attachment #589614 - Flags: review- → review+
(Assignee)

Updated

7 years ago
Depends on: 719697
(Assignee)

Updated

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

Updated

7 years ago
Attachment #589614 - Flags: checked-in+
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
Attachment #588543 - Attachment is obsolete: true
(Assignee)

Updated

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

Updated

7 years ago
Attachment #590803 - Flags: checked-in+
update summary to reflect new test type now covered by bug 719443
Summary: install robocop.apk and add a robocop test type for native android → install robocop.apk for native android
(Reporter)

Comment 23

7 years ago
is this done?  I see all patches are checked in?  Are they live?
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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Final changes to bug 719697 requires changes to the way robocop.apk installed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 592601 [details] [diff] [review]
download robocop.apk to foopy

2 changes: first to download robocop.apk, second to pick up robocop.ini file from new location.
Attachment #592601 - Flags: review?(aki)
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.
Attachment #592601 - Flags: review?(aki) → review-
(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.
(Reporter)

Comment 29

7 years ago
My understanding is we are done with this and waiting for everything else to be resolved before closing this bug?
(Reporter)

Updated

7 years ago
Blocks: 723667
(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.
Created attachment 594019 [details] [diff] [review]
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
Attachment #592601 - Attachment is obsolete: true
Attachment #594019 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #594019 - Flags: review? → review?(aki)
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 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
Attachment #594019 - Flags: review?(aki)
Attachment #594019 - Flags: review+
Attachment #594019 - Flags: checked-in+

Comment 34

7 years ago
Is this done?
reconfig deployed this today at 1300 PST. So done. (yay!)
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 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.