Closed
Bug 994195
Opened 10 years ago
Closed 10 years ago
Add tests for RawResource class
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
Attachments
(1 file, 2 obsolete files)
3.22 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
Summary says it all. Needs some work in the browser test's build system (see bug 994135).
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Will wait until bug 994135 is fixed to request review.
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8404076 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8404077 -
Attachment is obsolete: true
Comment 6•10 years ago
|
||
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+
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Blocks: suggested-sites-v1
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d95dd0d913f1
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d95dd0d913f1
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•