1015395 - Make testFindInPage more robust

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[:Margaret Leibovic]

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.

Comment 1

[:Margaret Leibovic]

We should fix this as part of the work for bug 1014113.

Comment 2

[Robin Ricard [:rricard]]

Taking it

Comment 3

[Robin Ricard [:rricard]]

Ok, so I'll start this patch based on my last patch in 1014113.

Comment 4

[Robin Ricard [:rricard]]

Wow ! Found the existence of robocop.ini ! I'm so stupid ! Now, I can work on this correctly duh !

Comment 5

[Robin Ricard [:rricard]]

Attachment: Make testFindInPage more robust

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.

Comment 6

[Robin Ricard [:rricard]]

Attachment: Here's the squashed version with the try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c3a9a92005d

Comment 7

[Robin Ricard [:rricard]]

Attachment: I updated the squashed patch with the latest fix in 1014113.

Comment 8

[Robin Ricard [:rricard]]

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 !)

Comment 9

[Robin Ricard [:rricard]]

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.

Comment 10

[Robin Ricard [:rricard]]

Attachment: With the isEmpty refinement...

Comment 11

[Robin Ricard [:rricard]]

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

Comment 12

[:Margaret Leibovic]

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

Comment 13

[Robin Ricard [:rricard]]

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 !

Comment 14

[Robin Ricard [:rricard]]

Attachment: So I started working on something new, it's surely not running (made the mistake to pull the repo, now I must recompile and it takes oddly lot of time...) but I'd like a little feedback on my approach (for example: is it better to use ES6 Promises+Generat

Comment 15

[Robin Ricard [:rricard]]

Attachment: Make testFindInPage more robust

Use a JS based test
Use document.getSelection

Comment 16

[Robin Ricard [:rricard]]

Attachment: Make testFindInPage more robust

Use a JS based test relying on document.getSelection

Comment 17

[Robin Ricard [:rricard]]

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 18

[:Margaret Leibovic]

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

Comment 19

[Robin Ricard [:rricard]]

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.

Comment 20

[Robin Ricard [:rricard]]

Attachment: Make testFindInPage more robust

Use a JS based test relying on document.getSelection

Comment 21

[Robin Ricard [:rricard]]

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 !

Comment 22

[Robin Ricard [:rricard]]

It must be <EventDispatcher instance>.dispatchEvent(...) https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/EventDispatcher.java#141 I think.

Comment 23

[Robin Ricard [:rricard]]

That doesn't seem to be the right thing though, I'll see that soon ...

Comment 24

[Robin Ricard [:rricard]]

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.

Comment 25

[Robin Ricard [:rricard]]

Attachment: 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

Comment 26

[: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 27

[:Margaret Leibovic]

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.

Comment 28

[Robin Ricard [:rricard]]

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.

Comment 29

[Robin Ricard [:rricard]]

Attachment: Skip the test on 2.3

Comment 30

[Mark Capella [:capella]]

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

Comment 31

[Robin Ricard [:rricard]]

Ok, working on it and re triggering a build then ! I'll update my commit message too !

Comment 32

[Robin Ricard [:rricard]]

Attachment: Fixed the nits ! Here's the try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=e300cde2a0bc

Comment 33

[Mark Capella [:capella]]

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.

Comment 34

[Robin Ricard [:rricard]]

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 35

[:Margaret Leibovic]

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 - [e-mail]:31:58
10:40:17 INFO - [e-mail]: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.

Comment 36

[Mark Capella [:capella]]

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.

Comment 37

[Robin Ricard [:rricard]]

I changed it but I forgot to save my VIM buffer. I'm fixing it right away !

Comment 38

[Robin Ricard [:rricard]]

Attachment: Make testFindInPage more robust

Fixed it ! Here's the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ec94342c6d2

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/9293d858a06c

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9293d858a06c