Closed Bug 571865 Opened 14 years ago Closed 12 years ago

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


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)



(Reporter: jmaher, Assigned: adifire)




(1 file, 3 obsolete files)

* 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)
Attached patch Tests for on/off states (obsolete) — Splinter Review
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.
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:

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

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.
Attached patch more homepage checks (obsolete) — Splinter Review
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.
Attachment #453290 - Attachment is obsolete: true
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();;

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;
    // Close preferences;
    // Open preferences
    is(panelContainer.hidden, true, "Now Preferences pane must not be visible");;
    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;

When you have it updated, please ask for a review from :stechz or :mfinkle.
Attached patch Reduced code (obsolete) — Splinter Review
Patch done as suggested by previous comment from Joel Maher (:jmaher). Also lesser code by moving repetitive blocks to a single method.
Attachment #457313 - Attachment is obsolete: true
Attachment #457485 - Flags: review?(mark.finkle)
mfinkle, what is the eta on a review for this?
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
Attachment #457485 - Flags: review?(mark.finkle) → review-
Have replaced all event listeners with messageManager.
Attachment #457485 - Attachment is obsolete: true
Attachment #460482 - Flags: review?(mark.finkle)
Attachment #460482 - Attachment is patch: true
Attachment #460482 - Attachment mime type: application/octet-stream → text/plain
it seems like this is fixed and we should make sure that there is coverage for this (I believe it was commented out).
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?
(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.
Attachment #460482 - Flags: review?(mark.finkle)
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.
Closed: 12 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.