Closed
Bug 894062
Opened 12 years ago
Closed 12 years ago
Add tests for mozdevice
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ffledgling, Assigned: ffledgling)
Details
Attachments
(3 files, 5 obsolete files)
8.37 KB,
patch
|
wlach
:
feedback+
|
Details | Diff | Splinter Review |
7.90 KB,
patch
|
cmtalbert
:
feedback+
|
Details | Diff | Splinter Review |
17.79 KB,
patch
|
ffledgling
:
review+
|
Details | Diff | Splinter Review |
Add tests to Mozdevice to increase coverage
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee: nobody → ffledgling
Attachment #775959 -
Flags: feedback?(wlachance)
Comment 2•12 years ago
|
||
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.
Attachment #775959 -
Flags: feedback?(wlachance) → feedback+
Assignee | ||
Comment 3•12 years ago
|
||
The logcat tests are a little messy, so I'm open to other alternatives. Please take a look and tell me what you think!
Attachment #777345 -
Flags: feedback?(wlachance)
Assignee | ||
Comment 4•12 years ago
|
||
Added more tests. Please take a look and tell me what you think?
Attachment #777345 -
Attachment is obsolete: true
Attachment #777345 -
Flags: feedback?(wlachance)
Attachment #777540 -
Flags: feedback?(wlachance)
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.
Attachment #777540 -
Flags: feedback?(wlachance) → feedback+
Assignee | ||
Comment 6•12 years ago
|
||
Attaching combined patch for review.
Assignee | ||
Updated•12 years ago
|
Attachment #778139 -
Flags: review?(wlachance)
Comment 7•12 years ago
|
||
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.
Attachment #778139 -
Flags: review?(wlachance) → review-
Assignee | ||
Comment 8•12 years ago
|
||
I've tried to fix the patch with the changes suggested, please take a look and tell me if anything else is required. Thanks!
Attachment #778139 -
Attachment is obsolete: true
Attachment #779026 -
Flags: review?(wlachance)
Comment 9•12 years ago
|
||
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?
Attachment #779026 -
Flags: review?(wlachance) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Restructured test to not use hard-coded md5 hash.
Attachment #779026 -
Attachment is obsolete: true
Attachment #779324 -
Flags: review?(wlachance)
Updated•12 years ago
|
Attachment #779324 -
Flags: review?(wlachance) → review+
Comment 11•12 years ago
|
||
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•12 years ago
|
||
fixes for various issues found (no logcat, syntax error)
Comment 13•12 years ago
|
||
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?
Attachment #779324 -
Flags: review+ → review-
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #779324 -
Attachment is obsolete: true
Attachment #779393 -
Attachment is obsolete: true
Attachment #779487 -
Flags: review?(wlachance)
Assignee | ||
Updated•12 years ago
|
Attachment #779487 -
Flags: review?(wlachance) → review+
Comment 15•12 years ago
|
||
Tests pass on my machine and patch looks good. Pushed:
https://github.com/mozilla/mozbase/commit/e021ca1bd735410c12285db81ac1222ce5aab635
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•