Closed Bug 1125531 Opened 6 years ago Closed 6 years ago

Optimize robocop SelectionHandler test notification generators

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(firefox38 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: capella, Assigned: ahmedibrahimkhali, Mentored)

Details

Attachments

(3 files, 2 obsolete files)

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
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
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
Attached patch testSelectionHandler.html.patch (obsolete) — Splinter Review
Could you please take a look on that patch that can be applied on testSelectionHandler.html ?
Attachment #8558129 - Flags: feedback?(markcapella)
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.
Attachment #8558129 - Flags: feedback?(markcapella) → feedback+
Attachment #8559449 - Flags: feedback?(markcapella)
Attachment #8558129 - Attachment is obsolete: true
Assignee: nobody → ahmedibrahimkhali
Status: NEW → ASSIGNED
Attachment #8559449 - Flags: feedback?(markcapella) → feedback+
Attached patch testTextareaSelections.patch (obsolete) — Splinter Review
Attachment #8560035 - Flags: feedback?(markcapella)
Attachment #8560047 - Flags: feedback?(markcapella)
Attachment #8560035 - Attachment is obsolete: true
Attachment #8560035 - Flags: feedback?(markcapella)
Attachment #8560054 - Flags: feedback?(markcapella)
Attachment #8560047 - Flags: feedback?(markcapella) → feedback+
Attachment #8560054 - Flags: feedback?(markcapella) → feedback+
Let's push to the try server
https://tbpl.mozilla.org/?tree=Try&rev=6e51c2e5160d
Attachment #8559449 - Flags: review?(margaret.leibovic)
Attachment #8560047 - Flags: review?(margaret.leibovic)
Attachment #8560054 - Flags: review?(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!
Attachment #8559449 - Flags: review?(margaret.leibovic) → review+
Attachment #8560047 - Flags: review?(margaret.leibovic) → review+
Attachment #8560054 - Flags: review?(margaret.leibovic) → review+
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
(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 :)
(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!
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.