Closed Bug 1015395 Opened 6 years ago Closed 5 years ago

Make testFindInPage more robust

Categories

(Firefox for Android :: Testing, defect)

30 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38

People

(Reporter: Margaret, Assigned: rricard)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 11 obsolete files)

12.26 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
Right now testFindInPage compares pixels to see if a big red box was scrolled of the screen when some search text was found. I think we should look into doing this some other way, like using JS to check the actual selection.
We should fix this as part of the work for bug 1014113.
Blocks: 1014113
Blocks: 975155
Assignee: nobody → ricard.robin
Taking it
Status: NEW → ASSIGNED
Ok, so I'll start this patch based on my last patch in 1014113.
Wow ! Found the existence of robocop.ini ! I'm so stupid ! Now, I can work on this correctly duh !
Attached patch Make testFindInPage more robust (obsolete) — Splinter Review
Change the layout of the robocop text page
Two levels of assertion with the green layout part
This patch depends on 1014113

I'll upload the complete patch (squashed with 1014113) and push it to try soon.
Attachment #8553766 - Flags: feedback?(margaret.leibovic)
Attachment #8553774 - Attachment is obsolete: true
Anyway, sorry for the overall waiting time for this patch series. I had a lot of work lately and I had lots of issues related to my tooling (between my stupid phone not able to connect to adb and the various tooling fails I had). I am finally ready to work better on FxAndroid (between the integration of robocop inside mach or WebIDE, many things have improved for the best ! and I have finally fixed everything: I run a genymotion vm so I have a fast and efficient debug process !)
About this new test: for now it's not really completely different from before. I'm trying to see if I can fix the intermittent problem with just a refactoring of the test's html.
Attached patch With the isEmpty refinement... (obsolete) — Splinter Review
Attachment #8553803 - Attachment is obsolete: true
Ok, now I see the core of the problem: I can't make any assumption on the device's screen size ... I knew that would happen but I think I was denying it. I'm going to think of a completely different approach for this. If you have an idea, I'd be grateful but I hope I'll be able to find it by myself before tomorrow
Thanks for the updates! I'm happy you were able to get your build environment fixed and now you can make try pushes :)

I'll clear feedback for now until you have a new version of the patch.

As I suggested in the bug description, I think it might be worth looking into seeing if we can get the selection with JS.

If you call window.getSelection() [1], you'll get a selection object you can use and compare.

I think you should consider rewriting testFindInPage to use JavascriptTest instead of PixelTest. The JS side of things will drive the logic of the test, and you can send a message to the Java part of the test to do the UI actions to actually invoke the find bar. testTrackingProtection is a good example to follow:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testTrackingProtection.java
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testTrackingProtection.js

[1] https://developer.mozilla.org/en-US/docs/Web/API/window.getSelection
Attachment #8553766 - Flags: feedback?(margaret.leibovic)
I finally catched again the start of the thread and indeed was looking to do it in JS. But that was still a little bit fuzzy on how to operate ! Thanks for the directions ! I think I'll be able to get something working by the end of your day I hope !
Attached patch Make testFindInPage more robust (obsolete) — Splinter Review
Use a JS based test
Use document.getSelection
Attachment #8554057 - Attachment is obsolete: true
Attachment #8554057 - Flags: feedback?(margaret.leibovic)
Attached patch Make testFindInPage more robust (obsolete) — Splinter Review
Use a JS based test relying on document.getSelection
Attachment #8554147 - Attachment is obsolete: true
Attachment #8554193 - Flags: feedback?(margaret.leibovic)
Here's the try related to the last patch, it works locally, let's see if it works on the tryserver ! https://treeherder.mozilla.org/#/jobs?repo=try&revision=b95c82d122bf
Comment on attachment 8554193 [details] [diff] [review]
Make testFindInPage more robust

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

Great work! This is exactly what I had in mind :)

Once you have a final patch ready for review let's make another try run, and I can help you fire off a lot of re-triggers to make sure that there are no intermittent failures.

::: mobile/android/base/tests/testFindInPage.java
@@ +37,5 @@
>          }
>  
> +        if (event.equals("Content:CloseFindInPage")) {
> +            try {
> +                close.click();

Can we also add a check in here to make sure the find bar closed properly? Maybe we should send a message back to the JS side and check that there isn't a selection anymore.

::: mobile/android/base/tests/testFindInPage.js
@@ +11,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const TEST_URL = "http://mochi.test:8888/tests/robocop/robocop_text_page.html";
> +
> +function browserEventResolver(browser, eventType) {

Nit: I would call this something like promiseBrowserEvent to convey that this returns a promise.

@@ +24,5 @@
> +    do_print("Now waiting for " + eventType + " event from browser");
> +  });
> +}
> +
> +function do_openTabWithUrl(url) {

Nit: I would just make the helper functions all camelCase (no underscores).

@@ +36,5 @@
> +
> +function do_findInPage(browser, text, nrOfMatches) {
> +  let repaintPromise = browserEventResolver(browser, "MozAfterPaint");
> +  do_print("Send findInPageMessage: " + text + " nth: " + nrOfMatches);
> +  Messaging.sendRequest({ type: "Content:FindInPage", text: text, nrOfMatches: nrOfMatches });

I would make this message name something like "Test:FindInPage" to make it clear that this message is only used in testing (the "Content:SecurityChange" example I pointed to in testTrackingProtection is actually a real message that comes from our application code).
Attachment #8554193 - Flags: feedback?(margaret.leibovic) → feedback+
Ok, great, just a dumb question: I feel like sending a message to java is kind of synchronous (in fact I don't need to wait for the repaint but I do that to avoid possible intermittent bugs...). I mean that's not what I expected from the message system so there might be a gotcha that I didn't saw there. if it's indeed asynchronous, I didn't found how to send events back to JS either (spent a lot of time on both MXR/DXR and I feel like I found hundred different ways to do it) and I would like to know what's the best way to do this in this particular case (ie. robocop JS testing). Thanks for the feedback, I knew the naming was wrong, I was prepared to change it :). I send you a new patch when possible (in a few hours if everything plays well on my side). After that, can we also look at bug 1014113 ? I changed it recently too.
Flags: needinfo?(margaret.leibovic)
Attached patch Make testFindInPage more robust (obsolete) — Splinter Review
Use a JS based test relying on document.getSelection
Attachment #8554193 - Attachment is obsolete: true
Attachment #8554609 - Flags: review?(margaret.leibovic)
Ok, so I wasn't able to check if the selection has disappeared (it seems to not disappear visually speaking ...) but I'm manly stuck because I'm not able to ping back js from java... I'll look for it again !
It must be <EventDispatcher instance>.dispatchEvent(...) https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/EventDispatcher.java#141 I think.
That doesn't seem to be the right thing though, I'll see that soon ...
Ok, well, after reading a few files, it seems like indeed that the calls to java are done in sync. To avoid repaint related errors, I'll apply the same trick as before. It should work and we will be able to ensure the empty selection.
Flags: needinfo?(margaret.leibovic)
(In reply to Robin Ricard [rricard] from comment #25)
> Created attachment 8554770 [details] [diff] [review]
> This one seems to be really good, it works perfectly locally (and it's quite
> fast !). Here's the try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=52a16b8fc136

I retriggered the jobs that run testFindInPage, and it looks like there's a crash on 2.3 devices. However, I would be fine with us landing the test disabled on 2.3, since those devices seem to be more troublesome in general.

I'm not sure I understand all of your comments about Java/JS messages being synchronous. The messages are passed on the Gecko thread, but other stuff will be happening in the meantime on the Java UI thread, so you'll need to put async logic in place if you want the test to wait for a response. In your JS code, that can be either waiting for explicit messages from Java, or for other events (it looks like you're currently doing that with "MozAfterPaint").
Comment on attachment 8554770 [details] [diff] [review]
This one seems to be really good, it works perfectly locally (and it's quite fast !). Here's the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52a16b8fc136

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

This is looking good to me, but I'd like capella to take a look as well to give the final review.

::: mobile/android/base/tests/robocop.ini
@@ +34,5 @@
>  skip-if = android_version == "10" || processor == "x86"
>  [testDistribution]
>  [testDoorHanger]
>  [testFilterOpenTab]
> +[testFindInPage]

Let's add a skip-if after this line to disable this test on 2.3.
Attachment #8554770 - Flags: review?(markcapella)
Attachment #8554770 - Flags: review?(margaret.leibovic)
Attachment #8554770 - Flags: feedback+
Great, I'll add that ! That's all I wanted to know thanks. It felt synchronous but it wasn't, that's why it can't work without the MozAfterPaint wait.
Attached patch Skip the test on 2.3 (obsolete) — Splinter Review
Attachment #8554770 - Attachment is obsolete: true
Attachment #8554770 - Flags: review?(markcapella)
Attachment #8555863 - Flags: review?(markcapella)
Attachment #8555863 - Flags: review?(margaret.leibovic)
Comment on attachment 8555863 [details] [diff] [review]
Skip the test on 2.3

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

I like this! This worked fine on both my test phone and tablet devices. I've got a few style nits, but that's the worst I can do :-D

Remember to add r=margaret to your commit message.

::: mobile/android/base/tests/testFindInPage.java
@@ +12,5 @@
>  
> +import org.mozilla.gecko.EventDispatcher;
> +import org.mozilla.gecko.util.GeckoEventListener;
> +
> +import org.json.JSONException;

Looks like this isn't needed.

@@ +28,5 @@
> +    public void handleMessage(String event, final JSONObject message) {
> +        if (event.equals("Test:FindInPage")) {
> +            try {
> +                String text = message.getString("text");
> +                int nrOfMatches = Integer.parseInt(message.getString("nrOfMatches"));

use 'final' for vars text, nrOfMatches.

@@ +49,5 @@
> +    public void setUp() throws Exception {
> +        super.setUp();
> +
> +        EventDispatcher.getInstance().registerGeckoThreadListener(this, "Test:FindInPage");
> +        EventDispatcher.getInstance().registerGeckoThreadListener(this, "Test:CloseFindInPage");

You might optimize a little here and below like:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DoorHangerPopup.java?rev=e925a610e68d&mark=44-46,51-53#26

@@ +64,1 @@
>      public void findText(String text, int nrOfMatches){

There's a load of nits in here ... not your problem :)

::: mobile/android/base/tests/testFindInPage.js
@@ +26,5 @@
> +}
> +
> +function openTabWithUrl(url) {
> +  do_print("Going to open " + url);
> +  let BrowserApp = Services.wm.getMostRecentWindow("navigator:browser").BrowserApp;

let browserApp = foo

@@ +50,5 @@
> +
> +function assertSelection(document, expectedSelection = false, expectedAnchorText = false) {
> +  let sel = document.getSelection();
> +  if(!expectedSelection) {
> +    do_print("Assert empty selection");

add space between if (foo)

@@ +56,5 @@
> +  } else {
> +    do_print("Assert selection to be " + expectedSelection);
> +    do_check_eq(sel.toString(), expectedSelection);
> +  }
> +  if(expectedAnchorText) {

add space

@@ +62,5 @@
> +    do_check_eq(sel.anchorNode.textContent, expectedAnchorText);
> +  }
> +}
> +
> +add_task(function *testFindInPage() {

It looks like moz convention for generators is function* foo
Attachment #8555863 - Flags: review?(markcapella) → review+
Ok, working on it and re triggering a build then ! I'll update my commit message too !
Attachment #8555863 - Attachment is obsolete: true
Attachment #8555863 - Flags: review?(margaret.leibovic)
Attachment #8555950 - Flags: review?(markcapella)
Comment on attachment 8555950 [details] [diff] [review]
Fixed the nits ! Here's the try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=e300cde2a0bc

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

I assume you'd fix the nits, so we'll just carryover the r+ (You didn't have to actually request again). I'll pass this to margaret for you.
Attachment #8555950 - Flags: review?(markcapella) → review?(margaret.leibovic)
Ok, thanks ! I'm still not totally used to bz for now ! SO this is the kind of things to still expect from me !
Comment on attachment 8555950 [details] [diff] [review]
Fixed the nits ! Here's the try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=e300cde2a0bc

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

Oops, looks like there must be a typo. There's a failure on the try run:

10:40:17 WARNING - TEST-UNEXPECTED-FAIL | testFindInPage | resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js - Unexpected exception ReferenceError: BrowserApp is not defined, see following stack:
10:40:17 INFO - openTabWithUrl@testFindInPage.js:31:58
10:40:17 INFO - testFindInPage@testFindInPage.js:67:23
10:40:17 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:314:40
10:40:17 INFO - TaskImpl@resource://gre/modules/Task.jsm:275:3
10:40:17 INFO - createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
10:40:17 INFO - Task_spawn@resource://gre/modules/Task.jsm:164:12
10:40:17 INFO - _run_next_test@robocop_head.js:1118:9
10:40:17 INFO - do_execute_soon/<.run@robocop_head.js:439:9 

Please update your patch, and once we get a green try run we can land it!

::: mobile/android/base/tests/testFindInPage.js
@@ +26,5 @@
> +}
> +
> +function openTabWithUrl(url) {
> +  do_print("Going to open " + url);
> +  let browserApp = Services.wm.getMostRecentWindow("navigator:browser").BrowserApp;

I think what you want here is BrowserApp with a capital B.
Attachment #8555950 - Flags: review?(margaret.leibovic) → review-
Well, I seemed to have confused this. When I asked him to change his local var to browserApp, he forgot
to change the |BrowserApp.selectedTab.id| reference on the next line.
I changed it but I forgot to save my VIM buffer. I'm fixing it right away !
Attachment #8556429 - Flags: review?(margaret.leibovic)
Attachment #8556429 - Flags: review?(margaret.leibovic) → review+
Attachment #8555950 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9293d858a06c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Depends on: 1128287
You need to log in before you can comment on or make changes to this bug.