1125528 - Create abstract class for SelectionHandler tests

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

The three SelectionHandler tests [1][2][3], use a (mostly) common parent class. Let's abstract it down even further, and extend our tests from there.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testSelectionHandler.java
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testTextareaSelections.java
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testInputSelections.java

Comment 1

[Aanand shekhar roy]

Hey hii,I wish to fix this bug.How do i start?

Comment 2

[Mark Capella [:capella]]

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

Comment 3

[Aanand shekhar roy]

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.

Comment 4

[Aanand shekhar roy]

(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.

Comment 5

[Andy Pusch [:AndyP]]

Attachment: bug1125528_createAbstractSelectionHandlerTest.diff

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

Comment 6

[Andy Pusch [:AndyP]]

Attachment: bug1125528_createAbstractSelectionHandlerTest.diff -v2

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.

Comment 7

[Mark Capella [:capella]]

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 ?

Comment 8

[Andy Pusch [:AndyP]]

(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.

Comment 9

[Mark Capella [:capella]]

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

Comment 10

[Andy Pusch [:AndyP]]

Attachment: bug1125528_createAbstractSelectionHandlerTest.diff -v3

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.

Comment 11

[Mark Capella [:capella]]

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 ...

Comment 12

[Andy Pusch [:AndyP]]

Attachment: bug1125528_createAbstractSelectionHandlerTest.diff -v4

I've tweaked the nits. 

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a8589a6bd49

Comment 13

[:Margaret Leibovic]

Comment on attachment 8565929 [details] [diff] [review]
bug1125528_createAbstractSelectionHandlerTest.diff -v4

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

Looks good!

Comment 14

[Mark Capella [:capella]]

Andy, do you want to mark this checkin neded?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/94367d1c24b3