Closed Bug 778366 Opened 8 years ago Closed 8 years ago

Add working Android SUTAgent tests to mozbase/mozdevice

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mihneadb, Assigned: mihneadb)

References

Details

Attachments

(1 file, 6 obsolete files)

No description provided.
Assignee: nobody → mbalaur
Depends on: 778329
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).
Attachment #646786 - Flags: review?(mcote)
Attachment #646786 - Flags: review?(ctalbert)
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.
You're right, I'll remake the diff. Weird. Not sure if we use that .ini file.
Attached patch Add the tests (obsolete) — Splinter Review
fixed the diff
Attachment #646786 - Attachment is obsolete: true
Attachment #646786 - Flags: review?(mcote)
Attachment #646786 - Flags: review?(ctalbert)
Attachment #647228 - Flags: review?(mcote)
Attachment #647228 - Flags: review?(ahalberstadt)
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.
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?
Ok, sure!
Attached patch New folder, readme. (obsolete) — Splinter Review
Hope this is ok.
Attachment #647228 - Attachment is obsolete: true
Attachment #647228 - Flags: review?(mcote)
Attachment #647228 - Flags: review?(ahalberstadt)
Attachment #647267 - Flags: review?(mcote)
Added some more code in test_unzip to account for the changes in bug #778329.
Attachment #647267 - Attachment is obsolete: true
Attachment #647267 - Flags: review?(mcote)
Attachment #647417 - Flags: review?(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
Attachment #647267 - Flags: review-
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
Attached patch source files (obsolete) — Splinter Review
Attachment #647417 - Attachment is obsolete: true
Attachment #647417 - Flags: review?(mcote)
Attachment #647780 - Flags: review?(mcote)
Test files (binary patch). [Too big to attach on bugzilla]

http://people.mozilla.org/~mbalaur/suttestfiles.bin.patch
Attached patch no binaries (obsolete) — Splinter Review
Attachment #647780 - Attachment is obsolete: true
Attachment #647780 - Flags: review?(mcote)
Attachment #648491 - Flags: review?(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.
Attachment #648491 - Flags: review?(mcote) → review-
Issue with paths filed as bug 780245.
Attachment #648491 - Attachment is obsolete: true
Attachment #648866 - Flags: review?(mcote)
Blocks: 780245
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.
Attachment #648866 - Flags: review?(mcote) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.