489158 - --browser-chrome Mochitests on Fennec [preferences]

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

testdev item to add automated tests for Fennec to test the preferences tab.  Here is an initial list of testcases we should implement:

    * Verify panning of preference list
    * Verify on/off retains state
    * Verify preferences and text
    * Would be nice to verify core functionality

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

* Verify panning of preference list
1) pan page to the left to reveal right side bar
2) verify preferences icon is visible and not depressed
3) click on the preferences icon
4) verify page pans to preferences view
5) verify 3 buttons (addons, downloads, preferences)
6) verify preferences is depressed and others are not pressed
7) verify 4th button (back button) exists, is visible, and depressed
8) pan page down and up
9) verify no horizontal panning (left + right) or angle panning (just up down)
10) verify double click doesn't try to zoom
11) click back button
12) verify right control bar showing with preferences button visible, but not depressed


* Verify on/off retains state
1) navigate to preferences pane 
2) for each yes/no option (images, javascript, cookies, passwords):
2.1) click option
2.2) verify it isn't reset when clicking the back button
2.3) verify it is set by checking preferences
3) turn all off, verify 2.2 + 2.3
4) turn all on, verify 2.2 + 2.3
5) for the start page option, change this to each option and verify it is checked in the select list after clicking done.  
6) the "use current page" will add a custom page option and the title of the page under the "start page" header.  Test this with about:fennec, about:blank, about:config, fennec start, and various other web pages (no title, long title, non english title)
7) return to main page (click the back button in the preferences)

* Verify preferences and text
1) click preferences button to view preferences
2) for each preference
2.1) verify the text
2.2) the button/option type (yes/no-checkbox, select dialog, link to page, submit button)
2.3) verify the height of each field is the same
3) verify content and "privacy & security" sections are grey and the same height.
4) return to main page (click the back button in the preferences)

* Verify core functionality
1) click preferences button to view preferences
2) click "Go to page" and verify you are taken to about:fennec
3) turn off show images, javascript, cookies, passwords
4) return to main page
5) load a html page (you have to create this and check it in with the tests) which has images, javascript, cookies (set/read) and submit button for passwords
6) verify the images do not show
7) verify the javascript does not run
8) verify we cannot save cookies (but can read them?)
9) verify when submitting a form, we are not asked to remember the password, verify by starting to fill out the form again
10) return the preferences
11) turn on all preferences
12) click the back button (to return to main window)

*NOTE: this doesn't test the start page or the clear private data button.

Comment 2

[Aditya Rao (:adifire)]

Attachment: Test to verifying panning of preferences

This test will do the first part of the preferences tests, panning of the preferences pane

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

This is great and good looking.  One thing I would adjust is there are some hardcoded values in the page for scrolling.  It would be nice to make some global variables that would represent these or best case scenario find these relative to window size.

+		is([x.value,y.value],"147,0","The right sidebar must be invisible");
+		scrollbox.scrollTo(240,0);

I am thinking the 147 and 240 should be automatically determined or global variables.  These methods might be of use:
tileContainer.getBoundingClientRect();
window.innerHeight;

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #3)
> This is great and good looking.  One thing I would adjust is there are some
> hardcoded values in the page for scrolling.  It would be nice to make some
> global variables that would represent these or best case scenario find these
> relative to window size.
> 
> +        is([x.value,y.value],"147,0","The right sidebar must be invisible");
> +        scrollbox.scrollTo(240,0);

I don't particularly like comparing the Array to a string version of the Array. Maybe we could break the "is" test into 2 separate "is" tests?

Also, don't use TABs in the code. Just use 2 space indenting.

More comments soon...

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

some other comments:

> +		let prefsList = document.getElementById("prefs-list")
> +		let scrollbox = document.getAnonymousElementByAttribute(prefsList, "anonid", "main-box").boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);

You do this several time, but you should only do it once and cache the element.

> +        setTimeout(gCurrentTest.onSideBarReady,2000);

We really look down on uses setTimeout for any "real" timeouts. Using a timeout of zero is ok, when just waiting for events to cycle.

Another question: Why are you using .scrollTo to "pan" the list and not EventUtils.synthesizeMouse?

Comment 6

[Aditya Rao (:adifire)]

Attachment: Patch fix

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 449298 [details] [diff] [review]
Patch fix

>diff --git a/chrome/tests/browser_preferences_basic.js b/chrome/tests/browser_preferences_basic.js
>+// ------------------Verifying panning of preferences list----------------------
>+gTests.push({
>+  onPageLoad: function(){
>+    // check whether the right sidebar is invisible
>+		let controls = document.getElementById("controls-scrollbox");
>+    let x = {};
>+    let y = {};
>+    gCurrentTest._contentScrollbox.getPosition(x,y);
>+    ok((x.value==147 & y.value==0),"The right sidebar must be invisible","Got "+x.value+" "+y.value+", expected 147,0");
>+
hardcoded value of 147?  I am not sure where you get this.  Can we at least put a comment in to explain what 147 means?  Would it be possible to define this as an attribute of the window size?

>+    gCurrentTest._contentScrollbox.getPosition(x,y);
>+		ok((x.value==230 & y.value==0),"The right sidebar must be visible","Got "+x.value+" "+y.value+", expected 230,0");

same issue with the '230' defined here.

>+    // Check whether it is moved up to the correct view
>+    gCurrentTest._prefsScrollbox.getPosition(x,y);
>+		ok((x.value==0 & y.value==104),"Preferences pane is panned up","Got "+x.value+" "+y.value+", expected 0,104");

same here, what is '104'?

>+    gCurrentTest._contentScrollbox.getPosition(x,y);
>+    ok((x.value==230 & y.value==0),"The right sidebar is still visible","Got "+x.value+" "+y.value+", expected 230,0");

another '230'



Overall, I see this being a complete test case and covering a lot of ground!  Great work.  These are just tiny nitpicky things I would rather be fixed or documented.

I would like to see if :mfinkle has any additional feedback on this.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 449298 [details] [diff] [review]
Patch fix

Nice patch.

>+++ b/chrome/tests/browser_preferences_basic.js

>+function runNextTest() {

>+  else {
>+    // Cleanup. All tests are completed at this point
>+      finish();

Fix indent

>+// ------------------Verifying panning of preferences list----------------------
>+gTests.push({
>+  desc: "Test basic panning of Preferences",
>+  _currenttab : null,
>+	_contentScrollbox : document.getElementById("controls-scrollbox").boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject),
>+	_prefsScrollbox : document.getAnonymousElementByAttribute(document.getElementById("prefs-list"), "anonid", "main-box").boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject),
>+
>+  run: function(){
>+		this._currenttab = Browser.addTab("about:blank",true);
>+		function handleEvent() {
>+		    gCurrentTest._currenttab.browser.removeEventListener("load", handleEvent, true);
>+		    gCurrentTest.onPageLoad();
>+		  };
>+		this._currenttab.browser.addEventListener("load", handleEvent , true);
>+  },

You have TABs in many places in the file. Please change to 2 space indents.

>+  onPageLoad: function(){

>+    ok((x.value==147 & y.value==0),"The right sidebar must be invisible","Got "+x.value+" "+y.value+", expected 147,0");

We might need to figure out some way to handle the hard coded numbers better. Especially if it makes the tests fragile (break often on different machinces or devices), but we can test first.

>+		ok((x.value==230 & y.value==0),"The right sidebar must be visible","Got "+x.value+" "+y.value+", expected 230,0");

Same here. Does 230 == "panned all the way to bottom"? if so, let's use the real measurement we can get with getBoundingClientRect or scroll offsets.

>+    // check if preferences pane is invisble 
>+    todo_is(document.getElementById("panel-container").hidden,true, "Preferences panel is invisble");

Why "todo"?


Fix the TABs and let's see how this works on other people's machines.

Comment 9

[Aditya Rao (:adifire)]

The hardcoded values were the results of the getPosition function.

For eg. (147,0) is the value when the Fennec loads to show the home page, and both sidebars hidden.

230,0 is when the right sidebar is made visible. I tried matching it with getBoundingClientRect but it does not seem to work out.

>+    // check if preferences pane is invisble 
>+    todo_is(document.getElementById("panel-container").hidden,true, "Preferences panel is invisble");

I added a todo there because it doesn't show the expected result.

Comment 10

[Aditya Rao (:adifire)]

Attachment: patch updated with corrected tab spaces

Comment 11

[Aditya Rao (:adifire)]

Attachment: patch updated

Sorry for the previous updates, tabs are irritating.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 450046 [details] [diff] [review]
patch updated 


>+  _contentScrollbox : document.getElementById("controls-scrollbox").boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject),
>+  _prefsScrollbox : document.getAnonymousElementByAttribute(document.getElementById("prefs-list"), "anonid", "main-box").boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject),
>+

can you make sure these lines are no more than 80 characters wide.  you can split lines like this:
document.getAnonymousElementByAttribute(document.getElementById("prefs-list"), "anonid", "main-box")
.boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject)

^ I put a newline before the .boxObject


>+    ok((x.value==147 & y.value==0),"The right sidebar must be invisible","Got "+x.value+" "+y.value+", expected 147,0");
>+

I saw the comment in the bug about '147' coming from getBoundingClientRect.  At the very least, put a comment in the test case mentioning that.  I assume getBoundingClientRect doesn't return the same value all the time and that is why it wasn't working?

Lets just add a comment in the testcase for each hard coded value.

>+
>+    // Now check whether it is not panned right/left    
>+    // Move the preferences pane right
>+    EventUtils.synthesizeMouse(prefsList, prefsList.clientWidth/2, prefsList.clientHeight/2, { type: "mousedown" });
>+    EventUtils.synthesizeMouse(prefsList, prefsList.clientWidth/4, prefsList.clientHeight/2, { type: "mousemove" });
>+    EventUtils.synthesizeMouse(prefsList, prefsList.clientWidth/4, prefsList.clientHeight/2, { type: "mouseup" });
>+

can we move these blocks of 3 synthesizeMouse() calls into a function.  

>+
>+    //Check whether double-click allow zooming in the Preferences pane
>+    EventUtils.synthesizeMouse(prefsList, prefsList.clientWidth / 2, prefsList.clientHeight / 2, {});
>+    EventUtils.synthesizeMouse(prefsList, prefsList.clientWidth / 2, prefsList.clientHeight / 2, {});    

same with the double click zoom.  Would be good to start abstracting this stuff for future use:)

Comment 13

[Aditya Rao (:adifire)]

Attachment: Patch Updated

Changes made:
* Compared values with getBoundingClientRect for panning the main page.
* New functions for panning and double click

Comment 14

[Aditya Rao (:adifire)]

Attachment: Minor update

Comment 15

[Aditya Rao (:adifire)]

Attachment: Minor update 2

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 450146 [details] [diff] [review]
Minor update 2

I think this patch is good to go.  :mfinkle, can you review this and give your thumbs up/down?

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 450146 [details] [diff] [review]
Minor update 2

I changed:
* the todo_is -> is (it was failing with unpected_pass on mt machine)
* Removed the double click test. It wasn't right (you can't check bv.getZoomLevel) and was failing since the page loaded in the browser was not coincidently at a zoom of 1.

This patch only tests the first block of tests in comment 0. Will you file new bugs for the other tests?

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/db7aa064fd6c