Closed Bug 994195 Opened 6 years ago Closed 6 years ago

Add tests for RawResource class

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: lucasr, Assigned: lucasr)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Summary says it all. Needs some work in the browser test's build system (see bug 994135).
Will wait until bug 994135 is fixed to request review.
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.
Attachment #8406887 - Flags: review?(margaret.leibovic)
Attachment #8404076 - Attachment is obsolete: true
Attachment #8404077 - Attachment is obsolete: true
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.
Attachment #8406887 - Flags: review?(margaret.leibovic) → review+
(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.
Blocks: 999398
(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.
https://hg.mozilla.org/mozilla-central/rev/d95dd0d913f1
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.