778366 - Add working Android SUTAgent tests to mozbase/mozdevice

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Add the tests.

Equivalent github commit: https://github.com/mihneadb/mozbase/commit/20fbc459f7a4f1d4d1e07c071da29f09047b603b

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 646786 [details] [diff] [review]
Add the tests.

Didn't change indentation for the old files (that's why some are indented by 2 spaces instead of 4).

Comment 3

[Andrew Halberstadt [:ahal]]

For some reason the diff is reversed (files are getting removed). If we aren't using the SUTAgent.ini.old anymore, it should probably be removed. We'll still have it in revision history if we need it.

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

You're right, I'll remake the diff. Weird. Not sure if we use that .ini file.

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Add the tests

fixed the diff

Comment 6

[William Lachance (:wlach)]

Sorry to jump in here, but how and when are these tests supposed to be run? The normal convention is that tests put in a module's tests/ directory are there to test the python code in the module, not third-party software (SUTAgent in this case).

I don't mind having these tests in mozbase, but IMO they should go into a different directory and we should have some kind of explicit instructions on how to use them.

Comment 7

[Mark Côté [:mcote]]

Yeah good point. For now, they would have to be run manually since they require an agent to test against (and we aren't maintaining the Python one). Mihnea, can you (a) move them to another dir (sut_tests or something, maybe) and (b) add a README?

Comment 8

[Mihnea Dobrescu-Balaur (:mihneadb)]

Ok, sure!

Comment 9

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: New folder, readme.

Hope this is ok.

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: updated unzip test for dest_dir arg

Added some more code in test_unzip to account for the changes in bug #778329.

Comment 11

[Mark Côté [:mcote]]

Comment on attachment 647267 [details] [diff] [review]
New folder, readme.

As discussed on IRC, the binary test files are missing. Either add them, or make them autogenerated.

Also a couple other comments:

- The README should explain what adb forward does for those unfamiliar with it.
- We should add some sort of licensing headers, either the MPL or public domain (since they are tests cases), as per https://www.mozilla.org/MPL/license-policy.html

Comment 12

[Mark Côté [:mcote]]

A couple other comments:

- Since this is now found in mozdevice, it should be unnecessary to modify your PYTHONPATH if you have mozdevice installed. Instead, import devicemanager and devicemanagerSUT from mozdevice, and remove that line from the README.

- I'd love it if all the lines in the README were < 80 characters long. :D

Comment 13

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: source files

Comment 14

[Mihnea Dobrescu-Balaur (:mihneadb)]

Test files (binary patch). [Too big to attach on bugzilla]

http://people.mozilla.org/~mbalaur/suttestfiles.bin.patch

Comment 15

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: no binaries

Comment 16

[Mark Côté [:mcote]]

Comment on attachment 648491 [details] [diff] [review]
no binaries

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

Cool, they all pass for me! Looks great. I just have a few little comments below, some of which are just nits about the original tests. There's a bigger issue (paths) but I'll file that separately. I'm giving it an r- only because the --ip and --port values are actually ignored (see my comment below for dmunit.py), which is bad. :)

::: mozdevice/sut_tests/SUTAgent.ini
@@ +3,5 @@
> +PORT = 28001
> +HARDWARE = Android_device_3
> +POOL = GENERAL
> +
> +[Agent Config]

This file doesn't actually appear to be used anywhere. I deleted it and reran the tests, and everything passed as before.

::: mozdevice/sut_tests/dmunit.py
@@ +23,5 @@
> +        """ Override this if you want set-up code in your test."""
> +        return
> +
> +    def setUp(self):
> +        self.dm = devicemanagerSUT.DeviceManagerSUT('127.0.0.1')

This should use the ip and port variables. As it stands the tests will always connect to 127.0.0.1:20701 no matter what you pass into runtests.py. :)

::: mozdevice/sut_tests/genfiles.py
@@ +40,5 @@
> +    shutil.copyfile(root + 'mytext.txt', prefix + 'sub1/file1.txt')
> +    mkdir(prefix + 'sub1/sub1.1')
> +    shutil.copyfile(root + 'mytext.txt', prefix + 'sub1/sub1.1/file2.txt')
> +    mkdir(prefix + 'sub2')
> +    shutil.copyfile(root + 'mytext.txt', prefix + 'sub2/file3.txt')

I was going to say that it's better to use os.path() instead of building your own paths, in case these tests are run on Windows, but I just realized that all the tests use POSIX-style paths, so I will file a separate bug about this.

::: mozdevice/sut_tests/runtests.py
@@ +34,5 @@
> +
> +    testLoader = dmunit.DeviceManagerTestLoader(isTestDevice)
> +    for s in scripts:
> +        suite.addTest(testLoader.loadTestsFromModuleName(s))
> +    unittest.TextTestRunner(verbosity=2).run(suite)

As you mentioned, would be nice to clean up the generated files here.

@@ +38,5 @@
> +    unittest.TextTestRunner(verbosity=2).run(suite)
> +
> +
> +# Deal with the options
> +defaults = {}

This variable isn't used.

@@ +58,5 @@
> +
> +parser.add_option("--testDevice", action="store_true", dest="isTestDevice",
> +                  help="Specifies that the device is a local test agent", default=False)
> +
> +(options, args) = parser.parse_args()

Not sure why this is all done outside of a function, when we have a __name__ == "__main__" block below it and a main() function above it. Move to one of those.

I think I also mentioned that it would be good to read the IP and port from an environment variable, TEST_DEVICE (e.g. TEST_DEVICE=10.0.0.1:20701). --ip and --port would override them. And if neither is provided, default to 127.0.0.1:20701.

::: mozdevice/sut_tests/test-files/sutagentdata.ini
@@ +1,3 @@
> +[sutagent]
> +ip=10.250.2.13
> +port=20700

This isn't actually used, although test_prompt.py refers to it in another directory. As I commented there, I think it's silly, so I'd remove this file.

::: mozdevice/sut_tests/test_prompt.py
@@ +29,5 @@
> +          ip = cfg.get('sutagent', 'ip')
> +          port = cfg.get('sutagent', 'cmdport')
> +      except:
> +        print 'ERROR: You did not fill out the test-files/sutagentdata.ini file'
> +        self.fail('could not read config file')

This bit with sutagentdata.ini is superfluous, given that we have to have the IP & port configured in the DeviceManager for all the other tests. I'd take it all out.

Comment 17

[Mark Côté [:mcote]]

Issue with paths filed as bug 780245.

Comment 18

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: update according to feedback

Comment 19

[Mark Côté [:mcote]]

Comment on attachment 648866 [details] [diff] [review]
update according to feedback

Cool, looks great! Please commit & push to mozbase and post a link to the diff.

Comment 20

[Mihnea Dobrescu-Balaur (:mihneadb)]

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