758392 - Enforce test type in Robotium tests

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Brian Nicholson (:bnicholson)]

Attachment: Create abstract getTestType() method in BaseTest

Currently, every test is required to call setTestType() to initialize the asserter. This is error-prone if someone (like me) forgets to add this call to their test class. We can enforce this by making an abstract method that all test cases must implement.

Comment 1

[Geoff Brown [:gbrown]]

Comment on attachment 626974 [details] [diff] [review]
Create abstract getTestType() method in BaseTest

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

Looks fine. I think it's a good idea.

jmaher -- any concerns?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 626974 [details] [diff] [review]
Create abstract getTestType() method in BaseTest

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

Thanks.  Wasn't thinking too much when I created the talos|mochitest types originally, this is much cleaner.

Comment 3

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/551fb80a2dca

Comment 4

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ad724a8f7e - took me a long time and 10 retriggers to decide it had, but it broke roboprovider.

Comment 5

[Geoff Brown [:gbrown]]

I'm not sure what went wrong exactly, but here's a thought: testBrowerProviderPerf is *not* derived from BaseTest, so I think getTestType() will never be called...and that ought to break something. There may be other tests that don't derive from BaseTest - worth checking.

Comment 6

[Brian Nicholson (:bnicholson)]

(In reply to Geoff Brown [:gbrown] from comment #5)
> I'm not sure what went wrong exactly, but here's a thought:
> testBrowerProviderPerf is *not* derived from BaseTest, so I think
> getTestType() will never be called...and that ought to break something.
> There may be other tests that don't derive from BaseTest - worth checking.

The patch also makes the same changes to ContentProviderTest.java.in, which testBrowserProviderPerf does inherit from. I pushed to try without the content provider tests (https://tbpl.mozilla.org/?tree=Try&rev=1c59b35b7d71), and it passed...so there's definitely something wrong, but it's not clear what.

Here's the part I removed to make it pass try: http://pastebin.mozilla.org/1652147

Comment 7

[Geoff Brown [:gbrown]]

Attachment: Create abstract getTestType() method in BaseTest

I updated the original patch for bitrot and debugged. I found the problem with ContentProviderTest: its base class, AndroidTestCase, contains testAndroidTestCaseSetUpProperly which causes a 2nd call to setUp / tearDown. The second setUp calls setLogFile a second time, which deletes the old log file, clobbering test results. Solution: guard the mAsserter code to ensure it is only called once.

Comment 8

[Geoff Brown [:gbrown]]

Try run pending: https://tbpl.mozilla.org/?tree=Try&rev=9f42d540f794

Comment 9

[Brian Nicholson (:bnicholson)]

ContentProviderTest extends BaseTest after bug 750753, so that makes things easier. I removed the parts specific to ContentProviderTest.

Try results here: https://tbpl.mozilla.org/?tree=Try&rev=8ac722481c91

http://hg.mozilla.org/integration/mozilla-inbound/rev/829b0fa41e2e

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/829b0fa41e2e