1232984 - Make "Downloadable content" classes testable and add basic Robolectric/Mockito junit tests

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Thanks to nalexander's talk in Orlando I realized that we already have junit 4 tests using Robolectric. That's exactly what I wanted to test "Downloadable content" (+ Mockito): Fast tests that run locally and are independent from devices and robocop.

A bit of refactoring might be needed to write tests for the existing code.

The goal of this bug is to have a small set of basic tests so that I can extend the functionality in the follow-up bugs[1] without breaking things. After that the tests can be extended as needed.

[1] https://bugzilla.mozilla.org/showdependencytree.cgi?id=1197720&hide_resolved=1

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1232984 - Move functionality from DownloadContentService to testable *Action subclasses. r?rnewman

Review commit: https://reviewboard.mozilla.org/r/28123/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28123/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1232984 - Add basic set of tests for downloadable content. r?rnewman

Review commit: https://reviewboard.mozilla.org/r/28125/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28125/

Comment 3

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/28125/#review25237

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/AllTests.java:16
(Diff revision 1)
> +@RunWith(Suite.class)

What's the advantage of manually producing a suite like this?  Is this just for running everything in the IDE easily?  I generally run by directory/package for that.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/DownloadActionTest.java:23
(Diff revision 1)
> +import static org.mockito.Matchers.any;

Do you have an opinion on `import static org.mockito.Mockito.*`?

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/DownloadActionTest.java:44
(Diff revision 1)
> +        verify(action, never()).buildHttpClient();

Oh, snap, this is nifty.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/DownloadActionTest.java:50
(Diff revision 1)
> +        DownloadContent content = new DownloadContent.Builder().build();

Throughout: it can be tricky to understand what tests are really saying.  Consider commenting the flow.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/StudyActionTest.java:29
(Diff revision 1)
> +public class StudyActionTest {

We almost always name our test classes `Test*.java`.  Is there a reason to switch?  (Perhaps the IDE recognizes `*Test.java` or produces that by default?)

This commit is awesome.  Now to run this stuff in automation!

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nick Alexander :nalexander from comment #3)
> mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/AllTests.
> java:16
> (Diff revision 1)
> > +@RunWith(Suite.class)
> 
> What's the advantage of manually producing a suite like this?  Is this just
> for running everything in the IDE easily?  I generally run by
> directory/package for that.

Oh, "run by directory" is handy. Yeah, I only used that for running this subset of tests. No need for that anymore.


(In reply to Nick Alexander :nalexander from comment #3)
> mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/
> DownloadActionTest.java:23
> (Diff revision 1)
> > +import static org.mockito.Matchers.any;
> 
> Do you have an opinion on `import static org.mockito.Mockito.*`?

That's how I prefer to use it but I've disabled grouping imports in the IDE so that this does not happen to org.mozilla.gecko.* imports. In return AS now writes a separate statement for every static method import. I'm not sure if I can configure AS to behave differently depending on what I'm importing: I'm too used to the keyboard shortcuts for writing imports myself. :)


(In reply to Nick Alexander :nalexander from comment #3)
> mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/
> DownloadActionTest.java:50
> (Diff revision 1)
> > +        DownloadContent content = new DownloadContent.Builder().build();
> 
> Throughout: it can be tricky to understand what tests are really saying. 
> Consider commenting the flow.

Yeah, that's a problem. In bug 1209513 I started to move common setup tasks to static methods to make the actual test more readable. I assume that if I would have written the tests earlier then the code would have ended up more testable / isolated and the tests would have been more readable. Comments are probably a good idea at this stage.


(In reply to Nick Alexander :nalexander from comment #3)
> mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/
> StudyActionTest.java:29
> (Diff revision 1)
> > +public class StudyActionTest {
> 
> We almost always name our test classes `Test*.java`.  Is there a reason to
> switch?  (Perhaps the IDE recognizes `*Test.java` or produces that by
> default?)

No particular reason. I'll rename my tests to match the pattern.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8698920 [details]
MozReview Request: Bug 1232984 - Add basic set of tests for downloadable content. r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28125/diff/1-2/

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

I updated the patches:
* The test files are now named Test*.java
* All test methods now include a comment describing the scenario and what I'm trying to test.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8698920 [details]
MozReview Request: Bug 1232984 - Add basic set of tests for downloadable content. r?rnewman

https://reviewboard.mozilla.org/r/28125/#review25775

The most rubbery of rubber-stamps.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/ef788bd314a5b3751bbed352326d974c9941c3d3
Bug 1232984 - Move functionality from DownloadContentService to testable *Action subclasses. r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/bc3d23a032c22cf7659e5107d6c90eb356f5c36b
Bug 1232984 - Add basic set of tests for downloadable content. r=rnewman

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef788bd314a5
https://hg.mozilla.org/mozilla-central/rev/bc3d23a032c2

Comment 10

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/28123/#review27409

The most rubberstampy of rubberstamps.