994195 - Add tests for RawResource class

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Summary says it all. Needs some work in the browser test's build system (see bug 994135).

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Add method to get test's context (r=nalexander)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Add tests for RawResource (r=margaret)

Comment 3

[Lucas Rocha (:lucasr)]

Will wait until bug 994135 is fixed to request review.

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Add tests for RawResource (r=margaret)

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8406887 [details] [diff] [review]
Add tests for RawResource (r=margaret)

Worked around the current limitation in browser instrumentation tests by mocking Content and Resources. This is good enough for now.

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8406887 [details] [diff] [review]
Add tests for RawResource (r=margaret)

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

wfm, and a reasonable work-around given the circumstances.

::: mobile/android/tests/browser/junit3/res/raw/test.txt
@@ +1,1 @@
> +RAW

This is not used, correct?

::: mobile/android/tests/browser/junit3/src/tests/TestRawResource.java
@@ +14,5 @@
> +import java.io.IOException;
> +
> +import org.mozilla.gecko.util.RawResource;
> +
> +public class TestRawResource extends BrowserTestCase {

I'm a huge fan of explanatory class comments...

@@ +16,5 @@
> +import org.mozilla.gecko.util.RawResource;
> +
> +public class TestRawResource extends BrowserTestCase {
> +    private static final int RAW_RESOURCE_ID = 1;
> +    private static final String RAW_CONTENTS = "RAW";

Why on earth does RawResource.get return a String?  At least, let's make that getString.  Maybe follow-up.

@@ +18,5 @@
> +public class TestRawResource extends BrowserTestCase {
> +    private static final int RAW_RESOURCE_ID = 1;
> +    private static final String RAW_CONTENTS = "RAW";
> +
> +    private static class TestContext extends MockContext {

... especially when doing odd things like this.  So explain why you're doing this mocking, and drop the follow-up ticket in the comment too.

Comment 7

[:Margaret Leibovic]

(In reply to Nick Alexander :nalexander from comment #6)

Thanks for helping me review this patch.

> ::: mobile/android/tests/browser/junit3/res/raw/test.txt
> @@ +1,1 @@
> > +RAW
> 
> This is not used, correct?

AIUI, this is the value that is checked in the test ...

> @@ +16,5 @@
> > +import org.mozilla.gecko.util.RawResource;
> > +
> > +public class TestRawResource extends BrowserTestCase {
> > +    private static final int RAW_RESOURCE_ID = 1;
> > +    private static final String RAW_CONTENTS = "RAW";
> 
> Why on earth does RawResource.get return a String?  At least, let's make
> that getString.  Maybe follow-up.

... here.

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Nick Alexander :nalexander from comment #6)
> Comment on attachment 8406887 [details] [diff] [review]
> Add tests for RawResource (r=margaret)
> 
> Review of attachment 8406887 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> wfm, and a reasonable work-around given the circumstances.
> 
> ::: mobile/android/tests/browser/junit3/res/raw/test.txt
> @@ +1,1 @@
> > +RAW
> 
> This is not used, correct?

Nope, removed.

> ::: mobile/android/tests/browser/junit3/src/tests/TestRawResource.java
> @@ +14,5 @@
> > +import java.io.IOException;
> > +
> > +import org.mozilla.gecko.util.RawResource;
> > +
> > +public class TestRawResource extends BrowserTestCase {
> 
> I'm a huge fan of explanatory class comments...

Good point, added a class comment.

> @@ +16,5 @@
> > +import org.mozilla.gecko.util.RawResource;
> > +
> > +public class TestRawResource extends BrowserTestCase {
> > +    private static final int RAW_RESOURCE_ID = 1;
> > +    private static final String RAW_CONTENTS = "RAW";
> 
> Why on earth does RawResource.get return a String?  At least, let's make
> that getString.  Maybe follow-up.

Fair enough. Just optimizing for the only use case we have i.e. I don't want to duplicate the code to read a String from the InputStream everywhere. I'm fine with with renaming the method to getAsString() in a follow-up. Filed bug 999398.

> @@ +18,5 @@
> > +public class TestRawResource extends BrowserTestCase {
> > +    private static final int RAW_RESOURCE_ID = 1;
> > +    private static final String RAW_CONTENTS = "RAW";
> > +
> > +    private static class TestContext extends MockContext {
> 
> ... especially when doing odd things like this.  So explain why you're doing
> this mocking, and drop the follow-up ticket in the comment too.

Good point, done.

Comment 9

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/d95dd0d913f1

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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