Closed
Bug 571865
Opened 15 years ago
Closed 13 years ago
--browser-chrome test for fennec preferences on/off state
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: jmaher, Assigned: adifire)
References
Details
Attachments
(1 file, 3 obsolete files)
15.93 KB,
patch
|
Details | Diff | Splinter Review |
* 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)
Assignee | ||
Comment 1•15 years ago
|
||
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.
Reporter | ||
Comment 2•15 years ago
|
||
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.
Assignee | ||
Comment 3•15 years ago
|
||
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
Reporter | ||
Comment 4•15 years ago
|
||
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.
Assignee | ||
Comment 5•15 years ago
|
||
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)
Reporter | ||
Comment 6•15 years ago
|
||
mfinkle, what is the eta on a review for this?
Comment 7•15 years ago
|
||
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-
Assignee | ||
Comment 8•15 years ago
|
||
Have replaced all event listeners with messageManager.
Attachment #457485 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #460482 -
Flags: review?(mark.finkle)
Reporter | ||
Updated•15 years ago
|
Attachment #460482 -
Attachment is patch: true
Attachment #460482 -
Attachment mime type: application/octet-stream → text/plain
Reporter | ||
Comment 9•15 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #460482 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 12•13 years ago
|
||
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.
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•