Closed Bug 1125528 Opened 7 years ago Closed 7 years ago

Create abstract class for SelectionHandler tests

Categories

(Firefox for Android Graveyard :: Text Selection, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: capella, Assigned: AndyP, Mentored)

Details

(Whiteboard: [lang=java])

Attachments

(1 file, 3 obsolete files)

Mentor: markcapella
Whiteboard: [lang=java]
Hey hii,I wish to fix this bug.How do i start?
Hi aanand. Note that this is not labelled as a good-first-bug. If you're new to contributing, you might have trouble with this. If the description above doesn't give you a basic idea of what is required, you should look into other bugs to start contributing.

You'll need a good working knowledge of Java, own an Android device, and be able to build and test the Android version of Firefox for Mobile devices. (This is easier if you already have a source repo and can build Firefox for desktop).

A skilled first-time contributor should be able to finish this in a couple weeks. Others may take longer, as creating your local build and test environments the first time is a challenge of itself. See [4] for overall details, and the robocop section [5] for testing specifics.

Leave comments / questions here, or look for me in IRC as nick:capella, in #introduction and #mobile channels.


[4] https://wiki.mozilla.org/Mobile/Fennec/Android
[5] https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop
Thanks Mark,Its embarassing but im little stuckup in using IRC,I am really naive so can we talk somewhere else instead of IRC.I am sorry.
(In reply to Mark Capella [:capella] from comment #2)
> Hi aanand. Note that this is not labelled as a good-first-bug. If you're new
> to contributing, you might have trouble with this. If the description above
> doesn't give you a basic idea of what is required, you should look into
> other bugs to start contributing.
> 
> You'll need a good working knowledge of Java, own an Android device, and be
> able to build and test the Android version of Firefox for Mobile devices.
> (This is easier if you already have a source repo and can build Firefox for
> desktop).
> 
> A skilled first-time contributor should be able to finish this in a couple
> weeks. Others may take longer, as creating your local build and test
> environments the first time is a challenge of itself. See [4] for overall
> details, and the robocop section [5] for testing specifics.
> 
> Leave comments / questions here, or look for me in IRC as nick:capella, in
> #introduction and #mobile channels.
> 
> 
> [4] https://wiki.mozilla.org/Mobile/Fennec/Android
> [5] https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop

Thanks Mark,Its embarassing but im little stuckup in using IRC,I am really naive so can we talk somewhere else instead of IRC.I am really sorry.
I successfully ran the three tests locally.
testSelectionHandler: http://pastebin.mozilla.org/8792006
testInputSelections: http://pastebin.mozilla.org/8792043
testTextareaSelections: http://pastebin.mozilla.org/8792044
Attachment #8565201 - Flags: review?(markcapella)
I forgot the public methods of the tests and had a wrong variable name in SelectionHandlerTest. Is there a specific way to test the public methods? The previous tests should not have worked, maybe I've forgotten something.
Attachment #8565201 - Attachment is obsolete: true
Attachment #8565201 - Flags: review?(markcapella)
Attachment #8565209 - Flags: review?(markcapella)
Assignee: nobody → 2013001
Status: NEW → ASSIGNED
Assignee: 2013001 → drag
Comment on attachment 8565209 [details] [diff] [review]
bug1125528_createAbstractSelectionHandlerTest.diff -v2

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

Looks good! For a final optimization, can we unify the individual geckoEventString values into a common "Robocop:testSelectionHandler" value. (Move const TYPE_NAME to SelectionUtil ?)

::: mobile/android/base/tests/SelectionHandlerTest.java
@@ +15,5 @@
> +
> +abstract class SelectionHandlerTest extends UITest {
> +
> +    final protected String geckoEventString;
> +    final protected String url;

final static private ?
Attachment #8565209 - Flags: review?(markcapella) → feedback+
(In reply to Mark Capella [:capella] from comment #7)
> Looks good! For a final optimization, can we unify the individual
> geckoEventString values into a common "Robocop:testSelectionHandler" value.
> (Move const TYPE_NAME to SelectionUtil ?)

I'm not exactly sure how the geckoEventString is used but if you say it's safe to use a single string for all of the tests I'll do it. :)
What do you mean by "move const TYPE_NAME to SelectionUtil"? Is TYPE_NAME the geckoEventString?

> ::: mobile/android/base/tests/SelectionHandlerTest.java
> @@ +15,5 @@
> > +
> > +abstract class SelectionHandlerTest extends UITest {
> > +
> > +    final protected String geckoEventString;
> > +    final protected String url;
> 
> final static private ?

If we make them final static we cannot assign them in the constructor. Making them private would mean we cannot use them in the extending classes if that's something that might be needed in the future.
Flags: needinfo?(markcapella)
Mmmmm ... i get my contributors mixed up ...

This will involve a small bit of javascript ...

Ping me in irc if this isn't enough of a clue
http://mxr.mozilla.org/mozilla-central/search?string=TYPE_NAME&case=1&find=mobile.*.&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Flags: needinfo?(markcapella)
I've hard-coded "Robocop:testSelectionHandler" into SelectionHandlerTest.java and SelectionUtils.js. I've also removed the TYPE_NAME definitions from the .html files and the geckoEventString constructor parameter from the .java files.
I have run the three tests locally and did not notice any error.
Attachment #8565209 - Attachment is obsolete: true
Attachment #8565470 - Flags: review?(markcapella)
Comment on attachment 8565470 [details] [diff] [review]
bug1125528_createAbstractSelectionHandlerTest.diff -v3

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

Let's wrap it up! r+ from me assuming you'll tweak the nits, push to try and post here, and r? to margaret for final say-so.

::: mobile/android/base/tests/SelectionHandlerTest.java
@@ +11,5 @@
> +
> +/**
> + * A base test class for selection handler tests.
> + */
> +

nit: unneeded blank line

@@ +13,5 @@
> + * A base test class for selection handler tests.
> + */
> +
> +abstract class SelectionHandlerTest extends UITest {
> +

nit: unneeded

@@ +14,5 @@
> + */
> +
> +abstract class SelectionHandlerTest extends UITest {
> +
> +    protected final String geckoEventString = "Robocop:testSelectionHandler";

private static final ...

@@ +15,5 @@
> +
> +abstract class SelectionHandlerTest extends UITest {
> +
> +    protected final String geckoEventString = "Robocop:testSelectionHandler";
> +    protected final String url;

private final ...
Attachment #8565470 - Flags: review?(markcapella) → review+
I've tweaked the nits. 

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a8589a6bd49
Attachment #8565470 - Attachment is obsolete: true
Attachment #8565929 - Flags: review?(margaret.leibovic)
Comment on attachment 8565929 [details] [diff] [review]
bug1125528_createAbstractSelectionHandlerTest.diff -v4

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

Looks good!
Attachment #8565929 - Flags: review?(margaret.leibovic) → review+
Andy, do you want to mark this checkin neded?
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/94367d1c24b3
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=java] → [lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/94367d1c24b3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][fixed-in-fx-team] → [lang=java]
Target Milestone: --- → Firefox 39
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.