Closed Bug 489158 Opened 11 years ago Closed 10 years ago

--browser-chrome Mochitests on Fennec [preferences]

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jmaher, Assigned: adifire)

References

Details

Attachments

(1 file, 6 obsolete files)

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
Blocks: 553474
* 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.
This test will do the first part of the preferences tests, panning of the preferences pane
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;
(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...
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?
Attached patch Patch fix (obsolete) — Splinter Review
Attachment #447565 - Attachment is obsolete: true
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.
Attachment #449298 - Flags: feedback?(mark.finkle)
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.
Attachment #449298 - Flags: feedback?(mark.finkle) → feedback+
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.
Attachment #449298 - Attachment is obsolete: true
Attached patch patch updated (obsolete) — Splinter Review
Sorry for the previous updates, tabs are irritating.
Attachment #450041 - Attachment is obsolete: true
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:)
Attached patch Patch Updated (obsolete) — Splinter Review
Changes made:
* Compared values with getBoundingClientRect for panning the main page.
* New functions for panning and double click
Attachment #450046 - Attachment is obsolete: true
Attached patch Minor update (obsolete) — Splinter Review
Attachment #450136 - Attachment is obsolete: true
Attached patch Minor update 2Splinter Review
Attachment #450143 - Attachment is obsolete: true
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?
Attachment #450146 - Flags: review?(mark.finkle)
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?
Attachment #450146 - Flags: review?(mark.finkle) → review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee: nobody → adicoolrao
You need to log in before you can comment on or make changes to this bug.