571865 - --browser-chrome test for fennec preferences on/off state

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

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

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

Comment 1

[Aditya Rao (:adifire)]

Attachment: Tests for on/off states

This test has coverage for steps 1-5.

There are several todo for step 6. 
- Way to find out whether the 4 option in homepage select list is selected when currentpage is set.

- The desc in the label for the Homepage does not turn up when selecting the currentpage.

Comment 2

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

overall I like this.  could we consolidate some of our code into the top.  I see a lot of these blocks (for each test case):
+    let panelOpenButton = document.getElementById("tool-panel-open");
+    let prefsHomeButton = document.getElementById("prefs-homepage-options");
+    let doneButton = document.getElementById("select-buttons-done");
+    let prefsClose = document.getElementById("tool-panel-close");
+    let homePageControl = document.getElementById("prefs-homepage");


If these were defined as global variables at the top, we could simplify the tests a bit.  Just for consideration.

Another area in testcase 1 that I see are a lot of calls like this:
document.getElementById("panel-container").hidden

Could we do something similar and make a:
var panelContainer = document.getElementById('panel-container');
.
:
panelContainer.hidden...


lastly on the home page tests, do we actually test if the homepage loads up as what we set it to?  I am thinking that would be a good end to end test of this.

Comment 3

[Aditya Rao (:adifire)]

Attachment: more homepage checks

There are more homepage checks although it fails for current page checks. Also done most of what was suggested in the previous comment by jmaher.

Comment 4

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

one thing I see is we make some global scoped variables, but assign them for each test.  I am going to contradict my previous comment, but I would rather see something like this:

function getPreferencesElements()
{
   var prefElements = {};
   prefElements.panelOpenButton = document.getElementById("tool-panel-open");
   return prefElements;
}

onPageLoad: function(){
  var prefs = getPreferencesElements();
  prefs.panelOpenButton.click();
  .
  :
}

Otherwise I think this is testing what we want.  There might be a few too many comments in the code:
    // Show Image check
    // see if it is on
    is(checkImage.checked,true,"Show images must be checked");    
    // turn it off
    checkImage.click();
    // Close preferences
    prefsClose.click();
    // Open preferences
    is(panelContainer.hidden, true, "Now Preferences pane must not be visible");
    panelOpenButton.click();
    is(panelContainer.hidden,false,"Preferences should be visible");
    // see if it is off
    is(checkImage.checked,false,"Show images must be unchecked");    
    // turn it on
    checkImage.click();


When you have it updated, please ask for a review from :stechz or :mfinkle.

Comment 5

[Aditya Rao (:adifire)]

Attachment: Reduced code

Patch done as suggested by previous comment from Joel Maher (:jmaher). Also lesser code by moving repetitive blocks to a single method.

Comment 6

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

mfinkle, what is the eta on a review for this?

Comment 7

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

Comment on attachment 457485 [details] [diff] [review]
Reduced code

>+    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);

The code does this a few times. Normally, I'd say "don't use about:blank to load a blank page", but you are using http://mochi.test pages, so it should be fine. If you were using chrome:// pages then I would suggest using chrome:// blank pages too. Note that since you are using http:// pages, addEventListener("load" shouldn't work for browser.tabs.remote=true

Why not use addMessageListener("pageload" ? I'm pretty sure others have seen that work in tests. Or even use the waitFor(..., pageLoaded(...)) pattern

nit: Please use spaces after commas in argument lists.

r- since we need to get the tests working for remote=true

Comment 8

[Aditya Rao (:adifire)]

Attachment: Changes- use of messageManager

Have replaced all event listeners with messageManager.

Comment 9

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

it seems like this https://bugzilla.mozilla.org/show_bug.cgi?id=578680 is fixed and we should make sure that there is coverage for this (I believe it was commented out).

Comment 10

[Andreea Pod]

Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110321 Firefox/4.0b13pre Fennec /4.1a1pre 
Device: Motorola Droid 2

Yes/No button is like a switch? I mean if I tap multiple times only on No it switches to yes and then back to no and so on, is this intended? or its a bug?

Comment 11

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

(In reply to comment #10)
> Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110321
> Firefox/4.0b13pre Fennec /4.1a1pre 
> Device: Motorola Droid 2
> 
> Yes/No button is like a switch? I mean if I tap multiple times only on No it
> switches to yes and then back to no and so on, is this intended? or its a bug?

That is intended. To is a separate bug for making the yes/no act like a slider.

Comment 12

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

Can we close this bug?  Right now we are not supporting xul based builds and we do not run browser-chrome on native android builds.