Robocop: Add test for Find in Page feature

RESOLVED FIXED in Firefox 22

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: AdrianT, Assigned: AdrianT)

Tracking

Trunk
Firefox 22
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

6 years ago
Posted patch concept test for Find in Page (obsolete) — Splinter Review
This test will track the progress of creating a Robocop testcase for the Find in Page feature.

The test assumes that the page is scrolled if a the user advances through a few Find matches and the page is not scrolled if there are no matches.

The attached concept uses a test page with areas 200px high which are colored in different colors. It first searches for a term that should not be found and tests that the page was not scrolled because the test pixel is red rgb(255,0,0). The second search does a search for "Robocop" which is present several times in the test page, it advances 2 times after the first match by tapping next and checks the color to be rgb(128,128,0).

I was unable to establish if the page was scrolled or not using:
- mDriver.getGeckoTop()
- mDriver.getScrollHeight() which seems to be 0 unless robocop does the scrolling
- startFrameRecording() and stopFrameRecording() since the output of stopFrameRecording is not O even if the page is not scrolled 
- with the methods waitForPaint and waitWithNoPaint since the app also seems to do Paint when the page is not scrolled.
(Assignee)

Updated

6 years ago
Depends on: 744859
(Assignee)

Updated

6 years ago
Blocks: 744859
No longer depends on: 744859
(Assignee)

Comment 1

6 years ago
Posted patch Find In Page patch v1 (obsolete) — Splinter Review
Attachment #692940 - Attachment is obsolete: true
(Assignee)

Comment 2

6 years ago
Comment on attachment 693832 [details] [diff] [review]
Find In Page patch v1

Cleaned up the patch and kept only the first div in the test html colored red. If the test finds the term in the page it will scroll it and the tested pixel will have a separate color.

I did not find a better way to check if the page is scrolled unless it is scrolled by robocop itself but if needed I can investigate more. Also there are a 2 sleeps that are need because the waitForTest is not enough
Attachment #693832 - Flags: review?(jmaher)
(Assignee)

Comment 3

6 years ago
> Cleaned up the patch and kept only the first div in the test html colored
> red. If the test finds the term in the page it will scroll it and the tested
> pixel will have a separate color.
I meant to say here: "the test pixel will be a different color "
Comment on attachment 693832 [details] [diff] [review]
Find In Page patch v1

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

I like the test in general, it fails each time I run it.  Why are we looking for pixel colors at specific points on the page?  I would think that different devices would react differently.

::: mobile/android/base/tests/testFindInPage.java.in
@@ +26,5 @@
> +        Actions.RepeatedEventExpecter paintExpecter = mActions.expectPaint();
> +        findText("no match should be found for this", 3);
> +        PaintedSurface painted = waitForPaint(paintExpecter);
> +        mAsserter.ispixel(painted.getPixelAt(width,height), 255, 0, 0, "Pixel at" + String.valueOf(width) + String.valueOf(height));
> +

I get a failure here on the tegra board each time I run this: 
6 INFO TEST-UNEXPECTED-FAIL | testFindInPage | Pixel at512139 - Color rgba(99,101,99,255) not close enough to expected rgb(255,0,0)

a couple things:
1) add a space after Pixel at
2) adjust it to be more like "Pixel at " + width + ", " + height <- notice the comma

@@ +48,5 @@
> +                      return false;
> +                  }
> +                }
> +            }, WAIT_FOR_TEST);
> +          mAsserter.ok(success, "looking for the next match button", "Found the next match button");

this assertion is not very descriptive, it might be confusing while tracking down the test.  Although I don't have a better suggestion right now.
Attachment #693832 - Flags: review?(jmaher) → review-
(Assignee)

Comment 5

6 years ago
(In reply to Joel Maher (:jmaher) from comment #4)
> Comment on attachment 693832 [details] [diff] [review]
> Find In Page patch v1
> 
> Review of attachment 693832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the test in general, it fails each time I run it.  Why are we looking
> for pixel colors at specific points on the page?  I would think that
> different devices would react differently.

I am looking at the color of the pixel because I added at the start of test page a div 200px high and 100% wide with red background and I am testing if the page is scrolled by checking if the pixel is still red. This is because I can't get the scroll position within the geko content unless the scroll is done by Robocop itself.

I will update the other changes and make the div higher and also i will move the height of the test pixel closer to GekoTop which should solve the fail and will no longer create issues with the device's size. 

If we manage to find a way to get the scroll height of geko directly we can change this method of checking for the color in order to see if the page is scrolled.
Comment on attachment 693832 [details] [diff] [review]
Find In Page patch v1

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

::: mobile/android/base/tests/testFindInPage.java.in
@@ +31,5 @@
> +        // Search that finds matches and therefor pans the page
> +        paintExpecter = mActions.expectPaint();
> +        findText("Robocop", 3);
> +        painted = waitForPaint(paintExpecter);
> +        mAsserter.ok(painted.getPixelAt(width,height) != Color.RED, "Checking pixel is not red","pixel is the correct color");

I think you should use ispixel here, rather than checking for a specific color. ispixel does a "fuzzy" compare, allowing for slight differences in color. Using != Color.RED may pass when the color is  red, but has been rendered just slightly differently.
(Assignee)

Comment 7

6 years ago
Hi Geoff,

Do you know if ispixel has a negative version to test that the color is not red since the pixel may be either black (text) or white (background)?
There is not a negative version of ispixel. 

How about:

   mAsserter.ok(!ispixel(pixel, Color.RED), ...

? Then if the pixel is red (or almost red), ispixel() is true, !ispixel is false and we assert. If the pixel is black, white, or anything but almost-red, ispixel() is false, !ispixel() is true, and the test passes. Is that what you want here?
(Assignee)

Comment 9

6 years ago
Yes this is what I wanted. I didn't realize that you can use ispixel like that without doing an mAsserter.ispixel. I will make the necessary adjustments here. Thanks.
(Assignee)

Comment 10

6 years ago
Posted patch Find In Page patch v1.1 (obsolete) — Splinter Review
Attachment #693832 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
Posted patch Find In Page patch v1.2 (obsolete) — Splinter Review
Cleaned up an extra space

Changes from the rejected patch:
 - Corrected a few assertion messages
 - Extended a few divs to 500px high to make sure the page scrolls a bit on a tablet in portrait mode
 - Changed the y coordonate of the test pixel from GekoHeight/5 to GekoHeight/8 to make sure the pixel first checks in the red div when for the first test the page is not scrolled
 - Created the ispixel method using the calculation algorithm from the ispixel method in the FennecMochitestAssert class. The method in the FennecMochitestAssert class returns void and there is no negative version of the test - a test that checks a pixel is not a specific color. This will add the "fuzzy" comparison of the pixel and ensure that it is not red and hence the page is scrolled.

I have continued to look for a better way to check if the page was scrolled or not but at the moment this is the only solution I could find. I will continue to check for a more appropriate solution.

Patch tested on the Acer Iconia Tab A500 running Android 3.1, Samsung Galaxy Tab 2 7.0 running Android 4.0.4 and HTC Desire running Android 2.2.
Attachment #697885 - Attachment is obsolete: true
Attachment #697890 - Flags: review?(jmaher)
this test fails for me on both tegra and panda platforms:
INFO | automation.py | Application pid: 0
0 INFO SimpleTest START
1 INFO TEST-START | testFindInPage
2 INFO TEST-PASS | testFindInPage | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_text_page.html should equal http://mochi.test:8888/tests/robocop/robocop_text_page.html
3 INFO TEST-PASS | testFindInPage | Looking for the next search match button in the Find in Page UI - Found the next match button
4 INFO TEST-PASS | testFindInPage | Checking if the next button was clicked - button was clicked
5 INFO TEST-PASS | testFindInPage | Checking if the next button was clicked - button was clicked
6 INFO TEST-UNEXPECTED-FAIL | testFindInPage | checking that painted surface loaded - painted surface loaded
Exception caught during test!
junit.framework.AssertionFailedError: 6 INFO TEST-UNEXPECTED-FAIL | testFindInPage | checking that painted surface loaded - painted surface loaded
	at junit.framework.Assert.fail(Assert.java:47)
	at org.mozilla.fennec_jmaher.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:107)
	at org.mozilla.fennec_jmaher.FennecMochitestAssert.ok(FennecMochitestAssert.java:136)
	at org.mozilla.fennec_jmaher.tests.PixelTest.waitForPaint(PixelTest.java:51)
	at org.mozilla.fennec_jmaher.tests.testFindInPage.testFindInPage(testFindInPage.java:28)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
	at org.mozilla.fennec_jmaher.tests.BaseTest.runTest(BaseTest.java:130)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
7 INFO TEST-UNEXPECTED-FAIL | testFindInPage | Exception caught - junit.framework.AssertionFailedError: 6 INFO TEST-UNEXPECTED-FAIL | testFindInPage | checking that painted surface loaded - painted surface loaded
8 INFO TEST-END | testFindInPage | finished in 72053ms
9 INFO TEST-START | Shutdown
10 INFO Passed: 4
11 INFO Failed: 2
12 INFO Todo: 0
13 INFO SimpleTest FINISHED
Comment on attachment 697890 [details] [diff] [review]
Find In Page patch v1.2

r- for failing tests.
Attachment #697890 - Flags: review?(jmaher) → review-
(Assignee)

Updated

6 years ago
Depends on: 828481
(Assignee)

Comment 14

6 years ago
Posted patch Find In Page patch v1.3 (obsolete) — Splinter Review
The old patch updated to the latest sources. Now that bug 828481 has been fixed this test works locally on Samsung Galaxy Tab 2 7.0 (Android 4.0.4).

The tryserver run had some problems on Android 4.0 with accessing the device and had to be restarted 4 times but eventualy was green:
https://tbpl.mozilla.org/?tree=Try&rev=18d50be71a9f
Attachment #697890 - Attachment is obsolete: true
Attachment #723936 - Flags: review?(jmaher)
Comment on attachment 723936 [details] [diff] [review]
Find In Page patch v1.3

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

good news is this passes on try server, bad news I have a few too many issues.

::: mobile/android/base/tests/testFindInPage.java.in
@@ +23,5 @@
> +        width = mDriver.getGeckoWidth()/2;
> +
> +        // Search that does not find the term and therefor should not pan the page
> +        Actions.RepeatedEventExpecter paintExpecter = mActions.expectPaint();
> +        findText("Robocoop", 3); // This will be close enough to existing text to test that seacrch finds just what it should

Robocop or Robocoop?, s/seacrch/search/

@@ +40,5 @@
> +        close = mDriver.findElement(getActivity(), "find_close");
> +          boolean success = waitForTest ( new BooleanTest() {
> +              public boolean test() {
> +                next = mDriver.findElement(getActivity(), "find_next");
> +                  if (next != null) {

this if clause doesn't need extra indentation.

@@ +43,5 @@
> +                next = mDriver.findElement(getActivity(), "find_next");
> +                  if (next != null) {
> +                      return true;
> +                  }
> +                  else {

please use "} else {"

@@ +52,5 @@
> +          mAsserter.ok(success, "Looking for the next search match button in the Find in Page UI", "Found the next match button");
> +
> +          // TODO: Find a better way to wait and then enter the text
> +          // Without the sleep this seems to work but the actions are not updated in the UI
> +          mSolo.sleep(2000);

does it really take 2 seconds to update the UI?  that seems wrong.  I would think 500 would be enough, but your testing probably uncovered we needed a higher number.

@@ +75,5 @@
> +          }
> +          close.click(); // Close find in page bar
> +    }
> +
> +    private boolean ispixel(int actual, int r, int g, int b) {

I don't like copying this from build/mobile/robocop/FennecMochitestAsserts.java.  I would prefer we refactor the original to be:

public boolean checkPixel(int actual, int r, int g, int b) {
...
}

public void ispixel(int actual, int r, int g, int b, String name) {
  boolean pass = checkPixel(actual, r, g, b)
  mAsserter.ok(pass, ...)
}
Attachment #723936 - Flags: review?(jmaher) → review-
(Assignee)

Comment 16

6 years ago
Posted patch Find In Page patch v1.4 (obsolete) — Splinter Review
Attachment #723936 - Attachment is obsolete: true
Attachment #724386 - Flags: review?(jmaher)
(Assignee)

Comment 17

6 years ago
(In reply to Joel Maher (:jmaher) from comment #15)
> Comment on attachment 723936 [details] [diff] [review]
> Find In Page patch v1.3
> 
> Review of attachment 723936 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> good news is this passes on try server, bad news I have a few too many
> issues.
> 
> ::: mobile/android/base/tests/testFindInPage.java.in
> @@ +23,5 @@
> > +        width = mDriver.getGeckoWidth()/2;
> > +
> > +        // Search that does not find the term and therefor should not pan the page
> > +        Actions.RepeatedEventExpecter paintExpecter = mActions.expectPaint();
> > +        findText("Robocoop", 3); // This will be close enough to existing text to test that seacrch finds just what it should
> 
> Robocop or Robocoop?, s/seacrch/search/
Here it's "Robocoop" because i'm looking for a text similar to the text on the page but which should not actually match - testing that search actually works

> @@ +52,5 @@
> > +          mAsserter.ok(success, "Looking for the next search match button in the Find in Page UI", "Found the next match button");
> > +
> > +          // TODO: Find a better way to wait and then enter the text
> > +          // Without the sleep this seems to work but the actions are not updated in the UI
> > +          mSolo.sleep(2000);
> 
> does it really take 2 seconds to update the UI?  that seems wrong.  I would
> think 500 would be enough, but your testing probably uncovered we needed a
> higher number.
Added only a 500ms sleep. Seems ok.

> @@ +75,5 @@
> > +          }
> > +          close.click(); // Close find in page bar
> > +    }
> > +
> > +    private boolean ispixel(int actual, int r, int g, int b) {
> 
> I don't like copying this from
> build/mobile/robocop/FennecMochitestAsserts.java.  I would prefer we
> refactor the original to be:
> 
> public boolean checkPixel(int actual, int r, int g, int b) {
> ...
> }
> 
> public void ispixel(int actual, int r, int g, int b, String name) {
>   boolean pass = checkPixel(actual, r, g, b)
>   mAsserter.ok(pass, ...)
> }
Just making a public function seemed to create problems accessing it. I created the isnotpixel assertion since in some other cases we might want to check that the background has changed -  or something similar. It may be good to have the choise of the negative assertion. 

Patch is green on try: https://tbpl.mozilla.org/?tree=Try&rev=17053fd99904
Comment on attachment 724386 [details] [diff] [review]
Find In Page patch v1.4

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

looking great, I just have one comment/question.

::: mobile/android/base/tests/testFindInPage.java.in
@@ +67,5 @@
> +                        return false;
> +                    }
> +                }
> +            }, WAIT_FOR_TEST);
> +            mSolo.sleep(2000); // TODO: Find a better way to wait here because waitForTest is not enough

can this be a 500 second sleep as well?
Attachment #724386 - Flags: review?(jmaher) → review+
(Assignee)

Comment 19

6 years ago
Sorry I missed that sleep. Chnaged to 500ms, done a few runs on the local Pandaboard with no issues encountered.
Attachment #724386 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
Can you please intrgrate the patch
https://hg.mozilla.org/mozilla-central/rev/4acd42c81df7
Assignee: nobody → adrian.tamas
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
You need to log in before you can comment on or make changes to this bug.