Optimize robocop SelectionHandler test notification generators

RESOLVED FIXED in Firefox 38

Status

()

Firefox for Android
Text Selection
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: capella, Assigned: Ahmed Khalil, Mentored)

Tracking

unspecified
Firefox 38
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
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
(Reporter)

Comment 2

3 years ago
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
(Assignee)

Comment 3

3 years ago
Created attachment 8558129 [details] [diff] [review]
testSelectionHandler.html.patch

Could you please take a look on that patch that can be applied on testSelectionHandler.html ?
Attachment #8558129 - Flags: feedback?(markcapella)
(Reporter)

Comment 4

3 years ago
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+
(Assignee)

Comment 5

3 years ago
Created attachment 8559449 [details] [diff] [review]
testSelectionHandler.patch
Attachment #8559449 - Flags: feedback?(markcapella)
(Reporter)

Updated

3 years ago
Attachment #8558129 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Assignee: nobody → ahmedibrahimkhali
Status: NEW → ASSIGNED
(Reporter)

Updated

3 years ago
Attachment #8559449 - Flags: feedback?(markcapella) → feedback+
(Assignee)

Comment 6

3 years ago
Created attachment 8560035 [details] [diff] [review]
testTextareaSelections.patch
Attachment #8560035 - Flags: feedback?(markcapella)
(Assignee)

Comment 7

3 years ago
Created attachment 8560047 [details] [diff] [review]
testInputSelections.patch
Attachment #8560047 - Flags: feedback?(markcapella)
(Assignee)

Comment 8

3 years ago
Created attachment 8560054 [details] [diff] [review]
testTextareaSelections.patch
Attachment #8560035 - Attachment is obsolete: true
Attachment #8560035 - Flags: feedback?(markcapella)
Attachment #8560054 - Flags: feedback?(markcapella)
(Reporter)

Updated

3 years ago
Attachment #8560047 - Flags: feedback?(markcapella) → feedback+
(Reporter)

Updated

3 years ago
Attachment #8560054 - Flags: feedback?(markcapella) → feedback+
(Reporter)

Comment 9

3 years ago
Let's push to the try server
https://tbpl.mozilla.org/?tree=Try&rev=6e51c2e5160d
(Reporter)

Updated

3 years ago
Attachment #8559449 - Flags: review?(margaret.leibovic)
(Reporter)

Updated

3 years ago
Attachment #8560047 - Flags: review?(margaret.leibovic)
(Reporter)

Updated

3 years ago
Attachment #8560054 - Flags: review?(margaret.leibovic)

Comment 10

3 years ago
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+

Updated

3 years ago
Attachment #8560047 - Flags: review?(margaret.leibovic) → review+

Updated

3 years ago
Attachment #8560054 - Flags: review?(margaret.leibovic) → review+
(Reporter)

Comment 11

3 years ago
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
(Assignee)

Comment 12

3 years ago
(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 :)
https://hg.mozilla.org/mozilla-central/rev/3a53cd9a4eb3
https://hg.mozilla.org/mozilla-central/rev/2789b6cd138d
https://hg.mozilla.org/mozilla-central/rev/986f47bf216b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38

Comment 14

3 years ago
(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!
You need to log in before you can comment on or make changes to this bug.