1125531 - Optimize robocop SelectionHandler test notification generators

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

The three SelectionHandler tests [1][2][3] use a common code pattern to trigger processing in SelectionHandler.js [4] where we directly grab a reference to the object and call to the desired method to be tested. (ex: [5]).

It would be much better to replace these |sh.observe(...)| references with |Services.obs.notifyObservers(...)| to fire the notifications. That would also test that SelectionHandler is actually listening as it should (and that nothing else is interfering).


[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/testSelectionHandler.html?rev=a914814d2054&force=1
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/testTextareaSelections.html?rev=c837afdde124&force=1
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/testInputSelections.html?rev=08ad468d5ed9&force=1

[4] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=260eb4adf6c6

[5] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/testSelectionHandler.html?rev=a914814d2054&force=1&mark=202-202,213-213#194

Comment 1

[Ahmed Khalil]

I'm interested in fixing this bug, I've seen the code that you've pointed out and I think I can do that.

I'm trying to understand the code, I'd like to know where should I find the Services.obs code ?

Lastly, from what I understood, I should just replace sh.observe with Services.obs.notifyObservers and supplying the correct parameters, right ?

Last question, how should I run a robocop test ? 
I tried running "./mach robocop testSelectionHandler" but I got this error [1]

[1] http://pastebin.com/QqaT1890

Comment 2

[Mark Capella [:capella]]

Hi Ahmed. Note that this is not labelled as a good-first-bug. If you're new to contributing, you might have trouble with this. It's not going to necessarily be a cut-and-paste situation. We'll know more as testing begins. I haven't thought it through entirely, but that's the plan, and if you can handle it, we welcome the help :-)

You'll need a good working knowledge of Javascript, own an Android device, and be able to build both the desktop and Android versions of Firefox, in order to test properly.

I estimate a skilled contributor could finish this from top to bottom in no less than two weeks. Creating your local build and test environments the first time is a challenge of itself. See [2.1] for Android build details, and the robocop section [2.2] for testing specifics.

re: |I should just replace sh.observe with Services.obs.notifyObservers and supplying the correct parameters, right ?|
That's the thought.

re: |how should I run a robocop test|
You may have neglected to build the desktop version of Firefox in order to have an xpcshell to provide a web server for the testing infrastructure. Or if you've done that, you may have neglected to inform the testing infrastructure where to find it via. an |EXPORT MOZ_HOST_BIN| command.

I'm sorry that the error message you saw was a little nebulous, we have a bug filed to make it friendlier. See bug 1126057 for further background.

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

[2.1] https://wiki.mozilla.org/Mobile/Fennec/Android
[2.2] https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop

Comment 3

[Ahmed Khalil]

Attachment: testSelectionHandler.html.patch

Could you please take a look on that patch that can be applied on testSelectionHandler.html ?

Comment 4

[Mark Capella [:capella]]

Comment on attachment 8558129 [details] [diff] [review]
testSelectionHandler.html.patch

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

This looks good in general. Can you re-generate the patch following the directions here?
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

If you format it properly, it should look something like this one. 
https://bugzilla.mozilla.org/attachment.cgi?id=8558275&action=edit

In that example, I'd used r=bnicholson on the commit message, but for yours, please use r=margaret. I'm going to provide you enough feedback to get you to where you can ask her for ?review, so she needs to be listed in your commit message.

When you repost the patch, flag me as ?feedback again. We'll work through the next two patches for testTextareaSelections and testInputSelections each in a different diff, and when all three are done, push them to the TRY server, and finally ask margaret to approve all three together.

Also, remember I mentioned in IRC yesterday that there are other devs working in this code and some of their changes might get approved ahead of yours causing you to have to rebase your patches locally as we go forward. I'll try to let you know if conflicts develop, but if you keep your local repo in-sync with m-c like daily, you should notice for yourself.

You can pop all you local patches and update from m-c by
hg qpop -a && hg pull -u

The re-push your patch back on
hg qpush

If you win up with .rej files, you'll have to do a quick manual rebase.

::: mobile/android/base/tests/roboextender/testSelectionHandler.html
@@ +201,4 @@
>  function testCloseSelection() {
>    var sh = getSelectionHandler();
>    var inputNode = document.getElementById("inputNode");
> +  var browserApp = Services.wm.getMostRecentWindow("navigator:browser").BrowserApp;

Please use 'let' instead of 'var'.
Fyi one of the other contributors is working on a patch to change all of these at once, so for now, just make sure your new line is right.

Comment 5

[Ahmed Khalil]

Attachment: testSelectionHandler.patch

Comment 6

[Ahmed Khalil]

Attachment: testTextareaSelections.patch

Comment 7

[Ahmed Khalil]

Attachment: testInputSelections.patch

Comment 8

[Ahmed Khalil]

Attachment: testTextareaSelections.patch

Comment 9

[Mark Capella [:capella]]

Let's push to the try server
https://tbpl.mozilla.org/?tree=Try&rev=6e51c2e5160d

Comment 10

[:Margaret Leibovic]

Comment on attachment 8559449 [details] [diff] [review]
testSelectionHandler.patch

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

Rubber stamp, thanks for handling the main review work, Capella!

Comment 11

[Mark Capella [:capella]]

Est completion time: 14 days
Act completion time:  8 days

First time contributor finishes under budget, -and- performs local robocop testing \o/

https://hg.mozilla.org/integration/fx-team/rev/986f47bf216b
https://hg.mozilla.org/integration/fx-team/rev/2789b6cd138d
https://hg.mozilla.org/integration/fx-team/rev/3a53cd9a4eb3

Comment 12

[Ahmed Khalil]

(In reply to Mark Capella [:capella] from comment #11)
> Est completion time: 14 days
> Act completion time:  8 days
> 
> First time contributor finishes under budget, -and- performs local robocop
> testing \o/
> 
> https://hg.mozilla.org/integration/fx-team/rev/986f47bf216b
> https://hg.mozilla.org/integration/fx-team/rev/2789b6cd138d
> https://hg.mozilla.org/integration/fx-team/rev/3a53cd9a4eb3

Thanks to you Capella, without you being very helpful I wouldn't finish that in time :)

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3a53cd9a4eb3
https://hg.mozilla.org/mozilla-central/rev/2789b6cd138d
https://hg.mozilla.org/mozilla-central/rev/986f47bf216b

Comment 14

[:Margaret Leibovic]

(In reply to Phil Ringnalda (:philor) from comment #13)
> https://hg.mozilla.org/mozilla-central/rev/3a53cd9a4eb3
> https://hg.mozilla.org/mozilla-central/rev/2789b6cd138d
> https://hg.mozilla.org/mozilla-central/rev/986f47bf216b

Awesome work! :)

In the future, you should make sure to give different commits unique commit messages that describe what's going on in the tests (I was a bit confused to see these same commit messages go by in the changelog).

And capella, you should also add r=capella to commit messages in the future when you help review them, so that you get credit where credit is due!