1393700 - Add test for parsing Pocket JSON response

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Chenxia Liu [:liuche]]

Splitting out from bug 1380808 - add a test for parsing JSON structure of Pocket response.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Bug 1393700 - Add test for parsing JSON response.

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

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8902039 [details]
Bug 1393700 - Add test for parsing JSON response.

https://reviewboard.mozilla.org/r/173444/#review179152

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:150
(Diff revision 1)
>              }
>          }
>      }
>  
> -    private static List<TopStory> jsonStringToTopStories(String jsonResponse) {
> +    @VisibleForTesting
> +    public static List<TopStory> jsonStringToTopStories(String jsonResponse) {

This can be package-private if you move the test to the same package.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/TestPocketJSONParsing.java:18
(Diff revision 1)
> +import org.mozilla.gecko.background.testhelpers.TestRunner;
> +
> +import java.util.List;
> +
> +@RunWith(TestRunner.class)
> +public class TestPocketJSONParsing {

nit: TestPocketStoriesLoader

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/TestPocketJSONParsing.java:19
(Diff revision 1)
> +
> +import java.util.List;
> +
> +@RunWith(TestRunner.class)
> +public class TestPocketJSONParsing {
> +    final String POCKET_URL = "POCKET_URL";

nit: private and static, here and below

Though, it ultimately doesn't matter. :P

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/TestPocketJSONParsing.java:24
(Diff revision 1)
> +    final String POCKET_URL = "POCKET_URL";
> +    final String PAGE_URL = "PAGE_URL";
> +    final String TITLE = "TITLE";
> +    final String IMG_URL = "IMAGE_URL";
> +
> +    final String BASIC_RESPONSE = "{\"status\":1, \"list\":[{" +

A thought: maybe it's better to construct the JSON via `JSONObject.toString` - it's more verbose and less readable but less error prone to update because of all that escaping.

This would not apply to the direct Pocket response.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/TestPocketJSONParsing.java:31
(Diff revision 1)
> +            "\"dedupe_url\":\"" + PAGE_URL + "\"," +
> +            "\"title\":\"" + TITLE + "\"," +
> +            "\"image_src\":\"" + IMG_URL + "\"}]}";
> +
> +    @Test
> +    public void testParsingWithBasicObject() {

nit: -> `testJSONStringToTopStoriesWithBasicObject`.

For unit tests, it's generally best to test a single method and so the method name should be in the test name.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/TestPocketJSONParsing.java:36
(Diff revision 1)
> +    public void testParsingWithBasicObject() {
> +        final List<TopStory> stories = PocketStoriesLoader.jsonStringToTopStories(BASIC_RESPONSE);
> +        Assert.assertEquals(stories.size(), 1);
> +
> +        final TopStory story = stories.get(0);
> +        Assert.assertEquals(story.getUrl(), PAGE_URL);

nit: Reverse the order of the arguments (here & below): JUnit assertions specify the expected argument first, then the actual argument: https://developer.android.com/reference/junit/framework/Assert.html#assertEquals(boolean,%20boolean)

I think this affects the way the test success/failure output is printed.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/TestPocketJSONParsing.java:46
(Diff revision 1)
> +    final String MISSING_TITLE_RESPONSE = "{\"status\":1, \"list\":[{" +
> +            "\"url\":\"" + POCKET_URL + "\"," +
> +            "\"dedupe_url\":\"" + PAGE_URL + "\"," +
> +            "\"image_src\":\"" + IMG_URL + "\"}]}";
> +    @Test
> +    public void testParsingWithMalformedObject() {

nit: What is malformed about this object? Maybe `testJSONStringToTopStoriesWithMissingTitle`

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/TestPocketJSONParsing.java:74
(Diff revision 1)
> +            "\"dedupe_url\":\"https:\\/\\/medium.com\",\"title\":\"Mediocrity is a Virus.\"," +
> +            "\"excerpt\":\"Little things become big things.\",\"domain\":\"medium.com\"," +
> +            "\"image_src\":\"https:\\/\\/d33ypg4xwx0n86.cloudfront.net\",\"published_timestamp\":\"1503578016\",\"sort_id\":2}]}";
> +
> +    @Test
> +    public void testParsingWithPocketObject() {

nit: -> `...WithRealPocketServerResponse`

Technically, the other objects you've crafted are "PocketObject"s too!

Comment 3

[Chenxia Liu [:liuche]]

Comment on attachment 8902039 [details]
Bug 1393700 - Add test for parsing JSON response.

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae9e6b6d3132
Add test for parsing JSON response. r=mcomella

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ae9e6b6d3132