Closed Bug 758392 Opened 9 years ago Closed 9 years ago

Enforce test type in Robotium tests

Categories

(Testing :: Mochitest, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla16

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #626974 - Flags: review?(gbrown)
Component: General → Mochitest
Product: Fennec Native → Testing
QA Contact: general → mochitest
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?
Attachment #626974 - Flags: review?(gbrown)
Attachment #626974 - Flags: review+
Attachment #626974 - Flags: feedback?(jmaher)
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.
Attachment #626974 - Flags: feedback?(jmaher) → feedback+
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.
Blocks: 756704
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.
(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
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.
Attachment #626974 - Attachment is obsolete: true
Attachment #635932 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/829b0fa41e2e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.