894062 - Add tests for mozdevice

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Anhad Jai Singh (:ffledgling)]

Add tests to Mozdevice to increase coverage

Comment 1

[Anhad Jai Singh (:ffledgling)]

Attachment: Work in Progress Patch for additional tests.

This patch adds all the tests that don't require root privileges to run.

Right now most of the tests are in their own file, this results in quite a few files, I'd like some advice regarding restructuring the tests to reduce the number of files.

Comment 2

[William Lachance (:wlach)]

Comment on attachment 775959 [details] [diff] [review]
Work in Progress Patch for additional tests.

This looks pretty good to me! I agree about the proliferation of files.

Comment 3

[Anhad Jai Singh (:ffledgling)]

Attachment: WIP patch 2nd set

The logcat tests are a little messy, so I'm open to other alternatives. Please take a look and tell me what you think!

Comment 4

[Anhad Jai Singh (:ffledgling)]

Attachment: WIP patch 2nd set

Added more tests. Please take a look and tell me what you think?

Comment 5

[cmtalbert]

Comment on attachment 777540 [details] [diff] [review]
WIP patch 2nd set

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

Taking over on this one from wlach, who's trying to rest and get well today. I think this looks good. As far as multiple files go, I don't really mind. I am of the school that it's better to have several files that clearly test a specific thing. This way tests both serve as simple tests and documentation on how to use the API.

Comment 6

[Anhad Jai Singh (:ffledgling)]

Attachment: Patch for mozdevice tests

Attaching combined patch for review.

Comment 7

[William Lachance (:wlach)]

Comment on attachment 778139 [details] [diff] [review]
Patch for mozdevice tests

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

Looks good! Would like to see some minor cleanup / changes before this lands, but nothing huge.

::: mozdevice/tests/sut_app.py
@@ +9,5 @@
> +
> +    def test_getAppRoot(self):
> +        command = [("getapproot org.mozilla.firefox",
> +                                    "/data/data/org.mozilla.firefox")]
> +

Could you align "/data/data/org.mozilla.firefox" with "getapproot" on the previous line?

::: mozdevice/tests/sut_fileMethods.py
@@ +17,5 @@
> +
> +        commands = [("hash /sdcard/test/file", "58f4a8369e17ab158171d13a1edfa99a")]
> +
> +        # Test Valid Hashes
> +        with mock.patch('mozdevice.DroidSUT._getLocalHash') as f:

I think using an actual local file here (as opposed to mock) might be more robust against the implementation changing.

@@ +43,5 @@
> +        commands = [("pull %s" % fname, "%s,%s\n%s" % (fname, len(content), content)),
> +                    # Hash is calculated and hardcoded
> +                    ("hash %s" % fname, "19aa46a1161d7262989b4a2439cdec60")]
> +
> +        fd, tmpfile = tempfile.mkstemp()

I would prefer if you used a NamedTemporaryFile here, like this:

https://github.com/mozilla/mozbase/blob/master/mozdevice/tests/sut_push.py#L23

This would eliminate the need for a separate cleanup step

::: mozdevice/tests/sut_logcat.py
@@ +26,5 @@
> +        "07-17 06:50:54.907 I/SUTAgentAndroid( 3876): Total Shared Dirty Memory          9216 kb\n\r"
> +        "07-17 06:55:21.627 I/SUTAgentAndroid( 3876): 127.0.0.1 : execsu /system/bin/logcat -v time -d dalvikvm:I "
> +        "ConnectivityService:S WifiMonitor:S WifiStateTracker:S wpa_supplicant:S NetworkStateTracker:S\n\r"
> +        "07-17 06:55:21.827 I/dalvikvm-heap( 3876): Grow heap (frag case) to 3.019MB for 102496-byte allocation\n\r"
> +        "return code [0]")

There is probably no reason to put this many lines in, but I guess it doesn't hurt. :)

@@ +35,5 @@
> +
> +        commands = [(inp, logcat_output)]
> +        m = MockAgent(self, commands=commands)
> +        d = mozdevice.DroidSUT("127.0.0.1", port=m.port logLevel=mozlog.DEBUG)
> +        self.maxDiff = None

I don't think this variable is used for anything?

::: mozdevice/tests/sut_remove.py
@@ +11,5 @@
> +    Deleting file(s) from /storage/emulated/legacy/Moztest
> +            <empty>
> +    Deleting directory /storage/emulated/legacy/Moztest
> +    """
> +

This comment about output is pretty confusing. I assume it's here by accident? In any case, would like to see it removed before this lands. It essentially just duplicates what we see below.

Comment 8

[Anhad Jai Singh (:ffledgling)]

Attachment: Updated patch with fixes

I've tried to fix the patch with the changes suggested, please take a look and tell me if anything else is required. Thanks!

Comment 9

[William Lachance (:wlach)]

Comment on attachment 779026 [details] [diff] [review]
Updated patch with fixes

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

Looks good! Some minor requests for cleanup with the hashing stuff in fileMethods. r+ with those addressed.

::: mozdevice/tests/sut_fileMethods.py
@@ +31,5 @@
> +            d = mozdevice.DroidSUT("127.0.0.1", port=m.port, logLevel=mozlog.DEBUG)
> +            self.assertTrue(d.validateFile('/sdcard/test/file', f.name))
> +
> +            # Test invalid hashes
> +            commands_invalid = [("hash /sdcard/test/file", "19aa46a1161d7262989b4a2439cdec60")]

Could you use some hash here that is more self-evidently invalid?

@@ +62,5 @@
> +                    ("ls", "file"),
> +                    ("isdir %s" % fname, "FALSE"),
> +                    ("pull %s" % fname, "%s,%s\n%s" % (fname, len(content), content)),
> +                    # Hash is calculated and hardcoded
> +                    ("hash %s" % fname, "19aa46a1161d7262989b4a2439cdec60")]

Could you calculate this hash dynamically?

Comment 10

[Anhad Jai Singh (:ffledgling)]

Attachment: 0003-Bug-894062-tests-for-mozdevice-s-devicemanager.patch

Restructured test to not use hard-coded md5 hash.

Comment 11

[William Lachance (:wlach)]

I'm having troubles when trying to actually run these tests: 

1. First, there was no manifest
2. There was a minor syntax error in the logcat test
3. The FileExistsTest is asserting:

test_onNonexistent (sut_fileExists.FileExistsTest) ... Exception in thread Thread-13:
Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 551, in __bootstrap_inner
    self.run()
  File "/usr/lib/python2.7/threading.py", line 504, in run
    self.__target(*self.__args, **self.__kwargs)
  File "/home/wlach/src/mozbase/mozdevice/tests/sut.py", line 61, in _serve_thread
    self.tester.assertEqual(data.strip(), command)
  File "/usr/lib/python2.7/unittest/case.py", line 511, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python2.7/unittest/case.py", line 504, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 'isdir' != 'isdir /'

(this is just what I found so far)

Comment 12

[William Lachance (:wlach)]

Attachment: mozdevice-tests-fixes.patch

fixes for various issues found (no logcat, syntax error)

Comment 13

[William Lachance (:wlach)]

Comment on attachment 779324 [details] [diff] [review]
0003-Bug-894062-tests-for-mozdevice-s-devicemanager.patch

Ok, given the issues below, I'm going to have to retract my r+. :) No worries, I'm sure we're almost there. Could you please try to fix the issues I mention, and make sure that the unit tests pass a full run on your machine?

Comment 14

[Anhad Jai Singh (:ffledgling)]

Attachment: 0001-Bug-894062-Add-tests-for-mozdevice-r-wlach.patch

Comment 15

[William Lachance (:wlach)]

Tests pass on my machine and patch looks good. Pushed:

https://github.com/mozilla/mozbase/commit/e021ca1bd735410c12285db81ac1222ce5aab635