Closed Bug 947914 Opened 10 years ago Closed 10 years ago

Add test for the functionality of Panel Menu buttons in Australis

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 35
Tracking Status
firefox29 --- wontfix

People

(Reporter: mihaelav, Assigned: mihaelav)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P-])

Attachments

(1 file, 19 obsolete files)

23.02 KB, patch
mihaelav
: review+
Details | Diff | Splinter Review
Create automated test for the following:

Steps:
1. Start Firefox
2. Open the Panel Menu
3. Click on every item in the Panel Menu

Expected result:
Each button should perform the right corresponding action
Assignee: nobody → mihaela.velimiroviciu
Mihaela, do you have a link to existent tests? Also which kind of framework you would like to use for those tests to write?
Flags: needinfo?(mihaela.velimiroviciu)
Existing tests: mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/
The test for this bug is also a browser chrome test.
Flags: needinfo?(mihaela.velimiroviciu)
Comment on attachment 8361023 [details] [diff] [review]
This tests the default panel menu buttons existence and functionality.

Review of attachment 8361023 [details] [diff] [review]:
-----------------------------------------------------------------

So, I think we should split these up in one test for each button. I also think this needs a try run before asking for review. Small nit below.

::: browser/components/customizableui/test/browser_947914_functionality_of_buttons.js
@@ +278,5 @@
> +
> +  yield closePanelMenu();
> +});
> +
> +function openPanelMenu() {

Why not just:

 yield PanelUI.show()

Wherever we use this?

It doesn't seem useful to check the panel's state and panelopen attribute every time.
Attachment #8361023 - Flags: review?(gijskruitbosch+bugs)
Attached patch v2 (obsolete) — Splinter Review
Updated according to previous feedback
Attachment #8361023 - Attachment is obsolete: true
Attachment #8363505 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8363505 [details] [diff] [review]
v2

Review of attachment 8363505 [details] [diff] [review]:
-----------------------------------------------------------------

Please consider this an r+ to land all the tests where I only had nits and/or comments on PanelUI.toggle() instead of PanelUI.hide().

For all the tests with more comments (full screen, preferences, cut/copy/paste, new (private) window, print, zoom controls), please can I see the updated patch first? :-)

::: browser/components/customizableui/test/browser_947914_button_addons.js
@@ +9,5 @@
> +  yield PanelUI.show();
> +
> +  let addonsButton = document.getElementById("add-ons-button");
> +  ok(addonsButton, "Add-ons button exists in Panel Menu");
> +  addonsButton.click();

Huh, I didn't know this was possible. Anyway, it seems to work...

@@ +12,5 @@
> +  ok(addonsButton, "Add-ons button exists in Panel Menu");
> +  addonsButton.click();
> +
> +  yield waitForCondition(function() gBrowser.selectedBrowser.contentWindow.
> +                                      document.title == "Add-ons Manager");

nit: I'd prefer gBrowser.currentURI && gBrowser.currentURI.spec == "about:addons"

::: browser/components/customizableui/test/browser_947914_button_copy.js
@@ +11,5 @@
> +  yield PanelUI.show();
> +
> +  let copyButton = document.getElementById("copy-button");
> +  ok(copyButton, "Copy button exists in Panel Menu");
> +  ok(copyButton.getAttribute("disabled"), "Copy button is initially disabled");

nit: is(copyButton.getAttribute("disabled"), "true", "Copy button is initially disabled");

@@ +18,5 @@
> +  gURLBar.value = testText;
> +  gURLBar.focus();
> +  gURLBar.select();
> +  yield PanelUI.show();
> +  ok(!copyButton.getAttribute("disabled"), "Copy button gets enabled");

nit: !copyButton.hasAttribute("disabled")

@@ +20,5 @@
> +  gURLBar.select();
> +  yield PanelUI.show();
> +  ok(!copyButton.getAttribute("disabled"), "Copy button gets enabled");
> +  copyButton.click();
> +  ok(gURLBar.value == testText, "Selected text is unaltered when clicking copy");

Ideally you should check here that the text was actually copied to the clipboard, with something along these lines (untested, based on the MDN pages we have for the clipboard.

let clipboard = Services.clipboard;
let globalClipboard = clipboard.kGlobalClipboard;
let transferable = Cc["@mozilla.org/widget/transferable;1"].createInstance("nsITransferable");
transferable.init(null);
transferable.addDataFlavor("text/unicode");
clipboard.getData(transferable, globalClipboard);
let str = {}, strLength = {};
transferable.getTransferData("text/unicode", str, strLength);
let clipboardValue = "";
if (str.value) {
  str.value.QueryInterface(Ci.nsISupportsString);
  clipboardValue = str.value.data;
}
is(clipboardValue, testText, "Expected data to be copied to the clipboard.");

@@ +25,5 @@
> +
> +  // clear the URL value and the clipboard
> +  gURLBar.value = "";
> +  let clipboard = Components.classes["@mozilla.org/widget/clipboardhelper;1"]
> +                                     .getService(Components.interfaces.nsIClipboardHelper);

You don't need this line

@@ +26,5 @@
> +  // clear the URL value and the clipboard
> +  gURLBar.value = "";
> +  let clipboard = Components.classes["@mozilla.org/widget/clipboardhelper;1"]
> +                                     .getService(Components.interfaces.nsIClipboardHelper);
> +  clipboard.copyString("");

... because you can just use: Services.clipboard.emptyClipboard(Services.clipboard.kGlobalClipboard);

(if you use the snippet above, you can obviously reuse the 'clipboard' and 'globalClipboard' variables declared there)

::: browser/components/customizableui/test/browser_947914_button_cut.js
@@ +21,5 @@
> +  yield PanelUI.show();
> +  ok(!cutButton.hasAttribute("disabled"), "Cut button gets enabled");
> +  cutButton.click();
> +  ok(gURLBar.value == "", "Selected text is removed from source when clicking on cut");
> +

Just like in the copy test, we should check that the data is actually on the clipboard, and the clearing of the clipboard should use the same pattern I noted there.

::: browser/components/customizableui/test/browser_947914_button_devtools.js
@@ +14,5 @@
> +  devtoolsButton.click();
> +  let devtoolsPanel = document.getElementById("PanelUI-developer");
> +  ok(devtoolsPanel.getAttribute("current"), "Developer tools Panel is in view");
> +
> +  yield PanelUI.toggle({type: "command"});

nit: yield PanelUI.hide();

::: browser/components/customizableui/test/browser_947914_button_fullscreen.js
@@ +15,5 @@
> +  yield waitForCondition(function() window.fullScreen);
> +  ok(window.fullScreen, "Fullscreen mode was opened");
> +
> +  // exit full screen mode
> +  window.fullScreen = !window.fullScreen;

This doesn't work on mac because it animates into fullscreen and this code gets all confused if you quickly revert it back to non-fullscreen. We should probably disable this test on mac for now.

@@ +16,5 @@
> +  ok(window.fullScreen, "Fullscreen mode was opened");
> +
> +  // exit full screen mode
> +  window.fullScreen = !window.fullScreen;
> +  yield waitForCondition(function() !document.mozFullScreen);

Err, you're using a different condition here. That's wrong. If window.fullScreen worked earlier, then it should work here, too.

::: browser/components/customizableui/test/browser_947914_button_history.js
@@ +14,5 @@
> +  historyButton.click();
> +  let historyPanel = document.getElementById("PanelUI-history");
> +  ok(historyPanel.getAttribute("current"), "History Panel is in view");
> +
> +  yield PanelUI.toggle({type: "command"});

Nit: yield PanelUI.hide(); (and the same everywhere where you're manually hiding the panel)

::: browser/components/customizableui/test/browser_947914_button_newPrivateWindow.js
@@ +17,5 @@
> +
> +        let win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> +        win.addEventListener("load", function newWindowHandler() {
> +          win.removeEventListener("load", newWindowHandler, false);
> +          is(win.location.href, "chrome://browser/content/browser.xul", "A new preferences window was opened");

Nit: this text is wrong, probably a copy-paste mistake?

@@ +28,5 @@
> +    }
> +  }
> +
> +  var windowWatcher = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
> +                                .getService(Components.interfaces.nsIWindowWatcher);

Services.ww provides this service; you don't need to do the long form here.

@@ +37,5 @@
> +  privateBrowsingButton.click();
> +
> +  yield waitForCondition(function() {
> +    return windowWasHandled == true;
> +  });

nit: yield waitForCondition(() => windowWasHandled);

Note that after this yield, you should clean up the observer even if the window wasn't fired, in case this test ever fails, otherwise weird things will happen. :-)

::: browser/components/customizableui/test/browser_947914_button_newWindow.js
@@ +27,5 @@
> +    }
> +  }
> +
> +  var windowWatcher = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
> +                                .getService(Components.interfaces.nsIWindowWatcher);

Same as for the other window opening test.

@@ +36,5 @@
> +  newWindowButton.click();
> +
> +  yield waitForCondition(function() {
> +    return windowWasHandled == true;
> +  });

And the same here, too.

::: browser/components/customizableui/test/browser_947914_button_paste.js
@@ +6,5 @@
> +
> +add_task(function() {
> +  info("Check paste button existence and functionality");
> +  let clipboard = Components.classes["@mozilla.org/widget/clipboardhelper;1"]
> +                                     .getService(Components.interfaces.nsIClipboardHelper);

Nit: please use Cc and Ci throughout to help make this more readable. You might even add a helper function to head.js to fetch the clipboardhelper, as it gets tedious to write/read this. :-)

@@ +8,5 @@
> +  info("Check paste button existence and functionality");
> +  let clipboard = Components.classes["@mozilla.org/widget/clipboardhelper;1"]
> +                                     .getService(Components.interfaces.nsIClipboardHelper);
> +
> + yield PanelUI.show();

Nit: indentation

@@ +28,5 @@
> +
> +  // close the current tab (because the url was altered), leaving the browser in about:home
> +  var tabToRemove = gBrowser.selectedTab;
> +  gBrowser.addTab("about:home");
> +  gBrowser.removeTab(tabToRemove);

about:home isn't the right thing to use here. Check what the tab's URL was previously (IIRC it's either about:blank or about:newtab) and use that, or it'll break tests further on in the test suite.

Also, you should clear the clipboard again here.

::: browser/components/customizableui/test/browser_947914_button_preferences.js
@@ +27,5 @@
> +    }
> +  }
> +
> +  var windowWatcher = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
> +                                .getService(Components.interfaces.nsIWindowWatcher);

And again.

@@ +36,5 @@
> +  preferencesButton.click();
> +
> +  yield waitForCondition(function() {
> +    return windowWasHandled == true;
> +  });

And again.

::: browser/components/customizableui/test/browser_947914_button_print.js
@@ +12,5 @@
> +  let printButton = document.getElementById("print-button");
> +  ok(printButton, "Print button exists in Panel Menu");
> +
> +  if(isOSX) {
> +    yield PanelUI.toggle({type: "command"});

yield PanelUI.hide();

@@ +23,5 @@
> +    // close print preview
> +    let printPreviewToolbarElements = document.getAnonymousNodes(document.getElementById("print-preview-toolbar"));
> +    ok(printPreviewToolbarElements, "Entered print preview mode");
> +
> +    //let printPreviewToolbarElements = document.getAnonymousNodes(document.getElementById("print-preview-toolbar"));

Please remove this line

@@ +24,5 @@
> +    let printPreviewToolbarElements = document.getAnonymousNodes(document.getElementById("print-preview-toolbar"));
> +    ok(printPreviewToolbarElements, "Entered print preview mode");
> +
> +    //let printPreviewToolbarElements = document.getAnonymousNodes(document.getElementById("print-preview-toolbar"));
> +    let closePrintPreview = printPreviewToolbarElements[printPreviewToolbarElements.length-2];

Holy hackman batman! How about:

PrintUtils.exitPrintPreview();

instead? :-)

::: browser/components/customizableui/test/browser_947914_button_savePage.js
@@ +8,5 @@
> +  info("Check save page button existence");
> +  yield PanelUI.show();
> +
> +  let savePageButton = document.getElementById("save-page-button");
> +  ok(savePageButton, "Save Page button exists in Panel Menu");

Hrm. I'm told you can technically deal with this dialog, but please file a followup for that; this patch is plenty complicated enough. :-)

@@ +10,5 @@
> +
> +  let savePageButton = document.getElementById("save-page-button");
> +  ok(savePageButton, "Save Page button exists in Panel Menu");
> +
> +  yield PanelUI.toggle({type: "command"});

yield PanelUI.hide();

::: browser/components/customizableui/test/browser_947914_button_zoomIn.js
@@ +14,5 @@
> +  zoomInButton.click();
> +  let pageZoomLevel = parseInt(ZoomManager.zoom * 100);
> +  let zoomResetButton = document.getElementById("zoom-reset-button");
> +  let expectedZoomLevel = parseInt(zoomResetButton.getAttribute("label"), 10);
> +  is(pageZoomLevel, expectedZoomLevel, "Page zoomed in correctly");

So this test doesn't actually check that the zoom level is what is expected. So if the button did nothing, and the reset button's label stayed unchanged, the test would pass. That should probably be fixed. Isn't the zoom level guaranteed to be 1.1 ?

@@ +17,5 @@
> +  let expectedZoomLevel = parseInt(zoomResetButton.getAttribute("label"), 10);
> +  is(pageZoomLevel, expectedZoomLevel, "Page zoomed in correctly");
> +
> +  // close the Panel
> +  yield PanelUI.toggle({type: "command"});

yield PanelUI.hide();

::: browser/components/customizableui/test/browser_947914_button_zoomOut.js
@@ +3,5 @@
> +  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +add_task(function() {
> +  info("Check zoom out button existence and functionality");

This test has the same issues as zoomIn.js

::: browser/components/customizableui/test/browser_947914_button_zoomReset.js
@@ +15,5 @@
> +
> +  zoomResetButton.click();
> +  let pageZoomLevel = parseInt(ZoomManager.zoom * 100);
> +  let expectedZoomLevel = parseInt(zoomResetButton.getAttribute("label"), 10);
> +  is(pageZoomLevel, expectedZoomLevel, "Page zoom reset correctly");

This should assert that the zoom level is now 1 again.

@@ +18,5 @@
> +  let expectedZoomLevel = parseInt(zoomResetButton.getAttribute("label"), 10);
> +  is(pageZoomLevel, expectedZoomLevel, "Page zoom reset correctly");
> +
> +  // close the panel
> +  yield PanelUI.toggle({type: "command"});

yield PanelUI.hide();
Attachment #8363505 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch v3 (obsolete) — Splinter Review
Updates the tests based on previous review, except for the savePage one for which I will log a separate bug and further investigate there.
Attachment #8363505 - Attachment is obsolete: true
Attachment #8364955 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8364955 [details] [diff] [review]
v3

Review of attachment 8364955 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there. For the next one, please make sure you've addressed all of the below comments, especially those that apply multiple times.

Annnd for the next one, can we have a try push? :-)

::: browser/components/customizableui/test/browser_947914_button_addons.js
@@ +22,5 @@
> +                            getElementById("addons-page");
> +  ok(addonsPage, "Add-ons page was opened");
> +
> +  // close the add-ons tab
> +  if(gBrowser.tabs.length > 1)

Nit: because the else block has braces, the if should, too.

::: browser/components/customizableui/test/browser_947914_button_copy.js
@@ +26,5 @@
> +  ok(gURLBar.value == testText, "Selected text is unaltered when clicking copy");
> +
> +  // check that the text was added to the clipboard
> +  let clipboard = Services.clipboard;
> +  let globalClipboard = clipboard.kGlobalClipboard;

nit: this should use const

@@ +44,5 @@
> +  is(clipboardValue, testText, "Data was copied to the clipboard.");
> +
> +  // clear the URL value and the clipboard
> +  gURLBar.value = "";
> +  Services.clipboard.emptyClipboard(Services.clipboard.kGlobalClipboard);

Nit: clipboard.emptyClipboard(globalClipboard);

::: browser/components/customizableui/test/browser_947914_button_cut.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +add_task(function() {
> +  info("Check cut button existence and functionality");

Same nits as for the copy button

::: browser/components/customizableui/test/browser_947914_button_newPrivateWindow.js
@@ +29,5 @@
> +    }
> +  }
> +
> +  var windowWatcher = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
> +                                .getService(Components.interfaces.nsIWindowWatcher);

You should remove this and use Services.ww throughout.

@@ +40,5 @@
> +  try{
> +    yield waitForCondition(() => windowWasHandled);
> +  }
> +  catch(e) {
> +    info("There were some errors while handling the new private window");

ok(false, "...");

::: browser/components/customizableui/test/browser_947914_button_newWindow.js
@@ +28,5 @@
> +    }
> +  }
> +
> +  var windowWatcher = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
> +                                .getService(Components.interfaces.nsIWindowWatcher);

Again, remove this, use Services.ww everywhere.

@@ +39,5 @@
> +  try{
> +    yield waitForCondition(() => windowWasHandled);
> +  }
> +  catch(e) {
> +    info("The new browser window was not handled.");

ok(false, "The new browser window was not handled.");

::: browser/components/customizableui/test/browser_947914_button_paste.js
@@ +27,5 @@
> +  is(gURLBar.value, text, "Text pasted successfully");
> +
> +  // close the current tab (because the url was altered), leaving the browser in about:home
> +  var tabToRemove = gBrowser.selectedTab;
> +  gBrowser.addTab("about:home");

You forgot to update this with initialLocation instead of about:home.

::: browser/components/customizableui/test/browser_947914_button_preferences.js
@@ +12,5 @@
> +
> +  let observer = {
> +    observe: function(aSubject, aTopic, aData) {
> +      if (aTopic == "domwindowopened") {
> +        windowWatcher.unregisterNotification(observer);

Services.ww throughout, please.

::: browser/components/customizableui/test/browser_947914_button_zoomIn.js
@@ +15,5 @@
> +  zoomInButton.click();
> +  let pageZoomLevel = parseInt(ZoomManager.zoom * 100);
> +  let zoomResetButton = document.getElementById("zoom-reset-button");
> +  let expectedZoomLevel = parseInt(zoomResetButton.getAttribute("label"), 10);
> +  ok(pageZoomLevel > 100 && pageZoomLevel == expectedZoomLevel, "Page zoomed in correctly");

You're implicitly assuming that initialPageZoom == 1. Make the assumption explicit by adding an is(initialPageZoom, 1, "Initial zoom factor should be 1") at the top.

::: browser/components/customizableui/test/browser_947914_button_zoomOut.js
@@ +14,5 @@
> +  zoomOutButton.click();
> +  let pageZoomLevel = parseInt(ZoomManager.zoom * 100);
> +  let zoomResetButton = document.getElementById("zoom-reset-button");
> +  let expectedZoomLevel = parseInt(zoomResetButton.getAttribute("label"), 10);
> +  ok(pageZoomLevel < 100 && pageZoomLevel == expectedZoomLevel, "Page zoomed out correctly");

You're implicitly assuming that initialPageZoom == 1. Make the assumption explicit by adding an is(initialPageZoom, 1, "Initial zoom factor should be 1") at the top.
Attachment #8364955 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch v4 (obsolete) — Splinter Review
Made the requested changes
Attachment #8364955 - Attachment is obsolete: true
Attachment #8365038 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8365038 [details] [diff] [review]
v4

Review of attachment 8365038 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the below nits addressed:

::: browser/components/customizableui/test/browser_947914_button_addons.js
@@ +24,5 @@
> +
> +  // close the add-ons tab
> +  if(gBrowser.tabs.length > 1) {
> +    gBrowser.removeTab(gBrowser.selectedTab);
> +  }

Nit: cuddly braces, so } else {, is prevailing style in this module.

::: browser/components/customizableui/test/browser_947914_button_copy.js
@@ +44,5 @@
> +  is(clipboardValue, testText, "Data was copied to the clipboard.");
> +
> +  // clear the URL value and the clipboard
> +  gURLBar.value = "";
> +  Services.clipboard.emptyClipboard(globalClipboard);

Nit: clipboard

::: browser/components/customizableui/test/browser_947914_button_cut.js
@@ +44,5 @@
> +  is(clipboardValue, testText, "Data was copied to the clipboard.");
> +
> +  // clear the URL value and the clipboard
> +  gURLBar.value = "";
> +  Services.clipboard.emptyClipboard(globalClipboard);

Nit: clipboard
Attachment #8365038 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #11)
> Try push:
> 
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=af8e3187db46

There was Linux orange on one of these for the print test. I don't understand why, but I've not looked in detail. Mihaela, can you take a look, please?

It's possible there's a problem with not making the PanelUI.hide() calls wait until the panel is hidden again... but that's the only thing I can think of offhand.
It doesn't reproduce locally on my machine (Ubuntu 13.04 32bit), whether I run the whole test folder or just the test alone.
Also tried on a local Ubuntu 12.04 32bit machine and there are no failures.
Status: NEW → ASSIGNED
Whiteboard: [Australis:P4]
Alright. So the try push is now a month old, I repushed, and then this should land assuming try is green:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=549e5f9e865b


Needinfo'ing so I don't forget.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #15)
> Alright. So the try push is now a month old, I repushed, and then this
> should land assuming try is green:
> 
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=549e5f9e865b
> 
> 
> Needinfo'ing so I don't forget.

All green, landed as:

remote:   https://hg.mozilla.org/integration/fx-team/rev/58ba97597854

Don't think we should uplift this.
Flags: needinfo?(gijskruitbosch+bugs) → in-testsuite+
I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/511de941ae39 for failing once on the first round of tests it had: https://tbpl.mozilla.org/php/getParsedLog.php?id=35010564&full=1&branch=fx-team#error0

There's several lines in that test's log output like this:
15:59:19     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_947914_button_print.js | Console message: [JavaScript Error: "A promise chain failed to handle a rejection.
which might be part of the problem, or maybe you just need to not timeout as quickly because debug builds are slow.
Flags: needinfo?(mihaela.velimiroviciu)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Wes Kocher (:KWierso) from comment #17)
> I had to back this out in
> https://hg.mozilla.org/integration/fx-team/rev/511de941ae39 for failing once
> on the first round of tests it had:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35010564&full=1&branch=fx-
> team#error0
> 
> There's several lines in that test's log output like this:
> 15:59:19     INFO -  TEST-INFO |
> chrome://mochitests/content/browser/browser/components/customizableui/test/
> browser_947914_button_print.js | Console message: [JavaScript Error: "A
> promise chain failed to handle a rejection.
> which might be part of the problem, or maybe you just need to not timeout as
> quickly because debug builds are slow.

No. There are some other promises which are rejected, and there are ours which fire when the next test runs (!) and then generate errors because the scope has been cleared by the test framework, so ok() and SimpleTest.whatever are all no longer defined.

The issue is that the panel never opens in this test. I suspect it might have to do with the previous test, which opens the preferences window, which might steal focus and cause the panel to close. We should probably wait there until the window is completely closed, and see if that helps.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: in-testsuite+
Attached patch v5 (obsolete) — Splinter Review
I added an observer for window closed in the preferences test. I hope this helps (I couldn't reproduce the issue from comment #17, neither before nor after this test fix).
Attachment #8365038 - Attachment is obsolete: true
Attachment #8379681 - Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(mihaela.velimiroviciu)
Comment on attachment 8379681 [details] [diff] [review]
v5

Review of attachment 8379681 [details] [diff] [review]:
-----------------------------------------------------------------

With these fixed, plus the previous ones, plus some try retriggers. I guess I'll just push both this patch and the previous one (also, both debug and opt) and doublecheck that this makes something go away.

::: browser/components/customizableui/test/browser.ini
@@ +8,5 @@
>  [browser_877006_missing_view.js]
>  [browser_877178_unregisterArea.js]
>  [browser_877447_skip_missing_ids.js]
>  [browser_878452_drag_to_panel.js]
> +[browser_878452_button_rss.js]

Umm. Unrelated hunk? Please to be getting rid of this.

::: browser/components/customizableui/test/browser_947914_button_preferences.js
@@ +8,5 @@
> +  info("Check preferences button existence and functionality");
> +  yield PanelUI.show();
> +
> +  var windowWasHandled = false;
> +

let prefWin = null;

@@ +12,5 @@
> +
> +  let observerWindowOpened = {
> +    observe: function(aSubject, aTopic, aData) {
> +      if (aTopic == "domwindowopened") {
> +        let win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);

prefWin = ...

@@ +26,5 @@
> +  }
> +
> +  let observerWindowClosed = {
> +    observe: function(aSubject, aTopic, aData) {
> +      if (aTopic == "domwindowclosed") {

&& aSubject == prefWin
Attachment #8379681 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch v6 (obsolete) — Splinter Review
Updated based on previous review
Attachment #8379681 - Attachment is obsolete: true
remote:   https://tbpl.mozilla.org/?tree=Try&rev=294b9c79667c


Retriggered opt and debug linux32 tests on the previous push to get an idea of how frequent this randomorange is so we know if this fixed it.
(In reply to :Gijs Kruitbosch from comment #22)
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=294b9c79667c
> 
> 
> Retriggered opt and debug linux32 tests on the previous push to get an idea
> of how frequent this randomorange is so we know if this fixed it.

So, this had none of the print failures on about the same sample size as the base push ( https://tbpl.mozilla.org/?tree=Try&rev=549e5f9e865b ) which had 2-3. However, it failed the _paste and _savePage tests instead...
Attached patch v7 (obsolete) — Splinter Review
Similar fix to the _print failures. Hopefully this fixes the timeout failures
Attachment #8379698 - Attachment is obsolete: true
Attachment #8382164 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8382164 [details] [diff] [review]
v7

Review of attachment 8382164 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Do we have any idea why the savePage test was failing, though? We should figure out what's up with that before landing this.

It might also be interesting to do a try push with the 3 window-opening tests removed, and see if that sees as much orange.
Attachment #8382164 - Flags: review?(gijskruitbosch+bugs) → review+
This patch doesn't contain the 3 tests which open new windows.
So, judging by the cautiously optimistic results of bug 976513's patch, I think what we should do here is make all the tests that open new windows use the same technique (yield promiseWindowClosed(windowReference)) and land that.

Mihaela, can you update the patch for that? :-)
Attached patch v8 (obsolete) — Splinter Review
(In reply to :Gijs Kruitbosch from comment #27)
> So, judging by the cautiously optimistic results of bug 976513's patch, I
> think what we should do here is make all the tests that open new windows use
> the same technique (yield promiseWindowClosed(windowReference)) and land
> that.
> 
> Mihaela, can you update the patch for that? :-)

Done
Attachment #8382164 - Attachment is obsolete: true
Attachment #8383110 - Attachment is obsolete: true
Attachment #8384560 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8384560 [details] [diff] [review]
v8

Review of attachment 8384560 [details] [diff] [review]:
-----------------------------------------------------------------

You don't need the close observer anymore if you're yielding for the promise.
Attachment #8384560 - Flags: review?(gijskruitbosch+bugs)
Attached patch patch (obsolete) — Splinter Review
Attachment #8384560 - Attachment is obsolete: true
Attachment #8385354 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8385354 [details] [diff] [review]
patch

Review of attachment 8385354 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming you only touched the newPrivateWindow, newWindow and preferences tests, r=me (I would have just interdiffed but bugzilla somehow doesn't like these patches)

Did you get checkin privileges yet or do I need to land this for you?
Attachment #8385354 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #31)
> Comment on attachment 8385354 [details] [diff] [review]
> patch
> 
> Review of attachment 8385354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Assuming you only touched the newPrivateWindow, newWindow and preferences
> tests, r=me (I would have just interdiffed but bugzilla somehow doesn't like
> these patches)
Yes, I've only touched those 3 tests.

> Did you get checkin privileges yet or do I need to land this for you?
I'm currently investigating and trying to fix the the access issues I got. Please land it for me. Thank you1
Backed out for various different browser-chrome failures (some are the new tests failing and others are due to them not cleaning up at the end of the test perhaps?):
https://tbpl.mozilla.org/php/getParsedLog.php?id=35599220&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=35601038&tree=Fx-Team
https://tbpl.mozilla.org/?tree=Fx-Team&rev=1d6730688107

remote:   https://hg.mozilla.org/integration/fx-team/rev/383867fd3bf6
Whiteboard: [Australis:P4] → [Australis:P-]
Let's try and land the bits that don't depend on opening a new window. Mihaela, can you provide a patch with just those tests and push it to try?
Flags: needinfo?(mihaela.velimiroviciu)
Actually, from what I investigated last week, it seems that the failures were caused by the fullscreen test. I am working at fixing that and trying to get a fixed patch soon.
Flags: needinfo?(mihaela.velimiroviciu)
(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #35)
> Actually, from what I investigated last week, it seems that the failures
> were caused by the fullscreen test. I am working at fixing that and trying
> to get a fixed patch soon.

We should just disable that test on OS X. I'm not aware of a way in which we can correctly detect that we've finished leaving fullscreen mode on that platform. :-(

Did this test also cause the issues on Windows?
Flags: needinfo?(mihaela.velimiroviciu)
(In reply to :Gijs Kruitbosch from comment #36)
> (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #35)
> > Actually, from what I investigated last week, it seems that the failures
> > were caused by the fullscreen test. I am working at fixing that and trying
> > to get a fixed patch soon.
> 
> We should just disable that test on OS X. I'm not aware of a way in which we
> can correctly detect that we've finished leaving fullscreen mode on that
> platform. :-(
This patch only checks the button availability on Mac, but doesn't test its functionality.
remote:   https://tbpl.mozilla.org/?tree=Try&rev=b69bb6c04872
 
> Did this test also cause the issues on Windows?
By removing the fullscreen test (completely), I get no oranges.
Attachment #8385354 - Attachment is obsolete: true
Attachment #8396332 - Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(mihaela.velimiroviciu)
Comment on attachment 8396332 [details] [diff] [review]
tests (fullscreen functionality not tested on Mac)

Review of attachment 8396332 [details] [diff] [review]:
-----------------------------------------------------------------

Looking at the interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8385354&action=interdiff&newid=8396332&headers=1


It looks like you changed a bunch of things compared to the old patch that could have stayed the same, I think, like the promiseWindowClosed stuff and yield PanelUI.show()...

The yield PanelUI.hide() *does* need changing, but here, too, you need to create the promise right before calling PanelUI.hide().
Attachment #8396332 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #38)
> Comment on attachment 8396332 [details] [diff] [review]
> tests (fullscreen functionality not tested on Mac)
>
> It looks like you changed a bunch of things compared to the old patch that
> could have stayed the same, I think, like the promiseWindowClosed stuff and
> yield PanelUI.show()...

Sorry, you're right, that was the wrong patch
Attachment #8397043 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8397043 [details] [diff] [review]
tests (fullscreen functionality not tested on Mac)

Review of attachment 8397043 [details] [diff] [review]:
-----------------------------------------------------------------

This looks much better, but I'm wondering about the fullscreen change. :-)

::: browser/components/customizableui/test/browser_947914_button_fullscreen.js
@@ +20,5 @@
> +    yield panelHiddenPromise;
> +  }
> +  else {
> +    fullscreenButton.click();
> +    promiseFullscreenChange(window);

You didn't include a yield keyword here, so it's not waiting for these events. Can you test if waiting for these events actually works? If so, we should probably do that, but if not, please get rid of these calls (and the promiseFullscreenChange function).
Attachment #8397043 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch partial patch (obsolete) — Splinter Review
This is a partial patch with tests which don't fail ATM. Will add the rest of the tests when fixed.

remote: https://tbpl.mozilla.org/?tree=Try&rev=a913ae8f52c1
Attachment #8396332 - Attachment is obsolete: true
Attachment #8397043 - Attachment is obsolete: true
Attachment #8406019 - Flags: review?(gijskruitbosch+bugs)
(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #41)
> Created attachment 8406019 [details] [diff] [review]
> partial patch
> 
> This is a partial patch with tests which don't fail ATM. Will add the rest
> of the tests when fixed.
> 
> remote: https://tbpl.mozilla.org/?tree=Try&rev=a913ae8f52c1

This try push is against quite an old cset (from march 31, afaict) - can you repush against a more current m-c tip, considering how much sadness this patch has already seen?
Comment on attachment 8406019 [details] [diff] [review]
partial patch

Review of attachment 8406019 [details] [diff] [review]:
-----------------------------------------------------------------

So this is r+ on everything but the cut/copy/paste & the devtools test. Do keep the nits and notes below in mind. I can land them for you if you can do a newer try run.

The devtools test I think by now is covered elsewhere, so that should be alright. The cut/copy/paste test... I'm a little worried about all this tab adding/removing. Is it really necessary?

::: browser/components/customizableui/test/browser_947914_button_cut.js
@@ +7,5 @@
> +add_task(function() {
> +  info("Check cut button existence and functionality");
> +
> +  var testText = "cut text test";
> +  let initialLocation = gBrowser.currentURI.spec;

Was there a particular reason we can't just reuse the same tab (and maybe reset the URL bar value, or use the search bar instead) ? Same question for the copy and paste tests...

::: browser/components/customizableui/test/browser_947914_button_devtools.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +add_task(function() {
> +  info("Check developer tools button existence and functionality");

I think this test has been superseded by http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_981305_separator_insertion.js, which tests a little bit more than this one (but no less). But please file a followup bug to add (still more) tests, e.g. to check that the items in the subview actually work. :-)

::: browser/components/customizableui/test/browser_947914_button_history.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +add_task(function() {
> +  info("Check history button existence and functionality");

Please file a followup bug to do more extensive testing of the subview's functionality. :-)

::: browser/components/customizableui/test/browser_947914_button_newPrivateWindow.js
@@ +17,5 @@
> +        privateWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> +        privateWindow.addEventListener("load", function newWindowHandler() {
> +          privateWindow.removeEventListener("load", newWindowHandler, false);
> +          is(privateWindow.location.href, "chrome://browser/content/browser.xul",
> +                                "A new browser window was opened");

Nit: make this line up (see note for newWindow.js)

::: browser/components/customizableui/test/browser_947914_button_newWindow.js
@@ +15,5 @@
> +    observe: function(aSubject, aTopic, aData) {
> +      if (aTopic == "domwindowopened") {
> +        newWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> +        newWindow.addEventListener("load", function newWindowHandler() {
> +          newWindow.removeEventListener("load", newWindowHandler, false);

Maybe check here that the window *isn't* private? :-)

@@ +17,5 @@
> +        newWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> +        newWindow.addEventListener("load", function newWindowHandler() {
> +          newWindow.removeEventListener("load", newWindowHandler, false);
> +          is(newWindow.location.href, "chrome://browser/content/browser.xul",
> +                                "A new browser window was opened");

nit: make this line up with the parenthesis at the beginning:

is(...
   "A new browser window was opened");

::: browser/components/customizableui/test/browser_947914_button_print.js
@@ +20,5 @@
> +    yield panelHiddenPromise;
> +  }
> +  else {
> +    printButton.click();
> +    yield waitForCondition(function() window.gInPrintPreviewMode);

Nit: make this use an arrow function too, like below.

::: browser/components/customizableui/test/browser_947914_button_zoomReset.js
@@ +20,5 @@
> +
> +  // close the panel
> +  let panelHiddenPromise = promisePanelHidden(window);
> +  PanelUI.hide();
> +  yield panelHiddenPromise;

Like the zoom out test, this test should reset to the initial zoom level if the reset click didn't do that (and assert it's 1 in the beginning of the test)
Attachment #8406019 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch patch (obsolete) — Splinter Review
(In reply to :Gijs Kruitbosch from comment #43)
> Comment on attachment 8406019 [details] [diff] [review]
> partial patch
> 
> Review of attachment 8406019 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So this is r+ on everything but the cut/copy/paste & the devtools test. Do
> keep the nits and notes below in mind. I can land them for you if you can do
> a newer try run.
> 
> The devtools test I think by now is covered elsewhere, so that should be
> alright. 
I removed the devtools test 

>The cut/copy/paste test... I'm a little worried about all this tab
> adding/removing. Is it really necessary?

I replaced the tab opening/removing with
gURLBar.value = initialLocation;

> ::: browser/components/customizableui/test/browser_947914_button_devtools.js
> @@ +4,5 @@
> > +
> > +"use strict";
> > +
> > +add_task(function() {
> > +  info("Check developer tools button existence and functionality");
> 
> I think this test has been superseded by
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> customizableui/test/browser_981305_separator_insertion.js, which tests a
> little bit more than this one (but no less). But please file a followup bug
> to add (still more) tests, e.g. to check that the items in the subview
> actually work. :-)
Logged bug 996505

> ::: browser/components/customizableui/test/browser_947914_button_history.js
> @@ +4,5 @@
> > +
> > +"use strict";
> > +
> > +add_task(function() {
> > +  info("Check history button existence and functionality");
> 
> Please file a followup bug to do more extensive testing of the subview's
> functionality. :-)
Logged bug 996506
 
> :::
> browser/components/customizableui/test/
> browser_947914_button_newPrivateWindow.js
> @@ +17,5 @@
> > +        privateWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> > +        privateWindow.addEventListener("load", function newWindowHandler() {
> > +          privateWindow.removeEventListener("load", newWindowHandler, false);
> > +          is(privateWindow.location.href, "chrome://browser/content/browser.xul",
> > +                                "A new browser window was opened");
> 
> Nit: make this line up (see note for newWindow.js)
Done

> ::: browser/components/customizableui/test/browser_947914_button_newWindow.js
> @@ +15,5 @@
> > +    observe: function(aSubject, aTopic, aData) {
> > +      if (aTopic == "domwindowopened") {
> > +        newWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> > +        newWindow.addEventListener("load", function newWindowHandler() {
> > +          newWindow.removeEventListener("load", newWindowHandler, false);
> 
> Maybe check here that the window *isn't* private? :-)
Done 

> @@ +17,5 @@
> > +        newWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> > +        newWindow.addEventListener("load", function newWindowHandler() {
> > +          newWindow.removeEventListener("load", newWindowHandler, false);
> > +          is(newWindow.location.href, "chrome://browser/content/browser.xul",
> > +                                "A new browser window was opened");
> 
> nit: make this line up with the parenthesis at the beginning:
> 
> is(...
>    "A new browser window was opened");
> 
> ::: browser/components/customizableui/test/browser_947914_button_print.js
> @@ +20,5 @@
> > +    yield panelHiddenPromise;
> > +  }
> > +  else {
> > +    printButton.click();
> > +    yield waitForCondition(function() window.gInPrintPreviewMode);
> 
> Nit: make this use an arrow function too, like below.
Done
 
> ::: browser/components/customizableui/test/browser_947914_button_zoomReset.js
> @@ +20,5 @@
> > +
> > +  // close the panel
> > +  let panelHiddenPromise = promisePanelHidden(window);
> > +  PanelUI.hide();
> > +  yield panelHiddenPromise;
> 
> Like the zoom out test, this test should reset to the initial zoom level if
> the reset click didn't do that (and assert it's 1 in the beginning of the
> test)
Done

remote: https://tbpl.mozilla.org/?tree=Try&rev=f6de9e70cdc5

carrying over the r+
Attachment #8406019 - Attachment is obsolete: true
Attachment #8406859 - Flags: review+
Comment on attachment 8406859 [details] [diff] [review]
patch

Review of attachment 8406859 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but please mind the following nits for the cut/copy and zoom reset tests. :-)

::: browser/components/customizableui/test/browser_947914_button_copy.js
@@ +24,5 @@
> +
> +  ok(!copyButton.hasAttribute("disabled"), "Copy button gets enabled");
> +
> +  copyButton.click();
> +  ok(gURLBar.value == testText, "Selected text is unaltered when clicking copy");

Nit: is(gURLBar.value, testText, "...");

::: browser/components/customizableui/test/browser_947914_button_cut.js
@@ +23,5 @@
> +  yield PanelUI.show();
> +
> +  ok(!cutButton.hasAttribute("disabled"), "Cut button gets enabled");
> +  cutButton.click();
> +  ok(gURLBar.value == "", "Selected text is removed from source when clicking on cut");

Nit: is(gURLBar.value, "", "...");

::: browser/components/customizableui/test/browser_947914_button_zoomReset.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +add_task(function() {
> +  info("Check zoom reset button existence and functionality");

This test should still assert that the zoom level is 1 when you start (because you're resetting it to 1 afterwards).
Attached patch partial patch (obsolete) — Splinter Review
Nits resolved
Attachment #8406859 - Attachment is obsolete: true
Attachment #8412537 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/cabda6c7f0e9
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [Australis:P-] → [Australis:P-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/cabda6c7f0e9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P-][fixed-in-fx-team] → [Australis:P-]
Target Milestone: --- → Firefox 31
Depends on: 1001754
Depends on: 1001755
Backed out for frequent Linux test failures (there are even more on fx-team that are unfiled). Going down fx-team today, I'm seeing at least one failure from these tests on nearly every push.

https://hg.mozilla.org/integration/fx-team/rev/efd35283d11d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 31 → ---
I was going to file a bug on this but was travelling.  When I updating my code base to sanity check before filing it was backed out.

In bug 992911 we are running tests as a single directory.  With that said, I see a lot of timeouts on linux (opt + debug), here are the tests that I saw timeout:
button_addons.js
button_copy.js
button_cut.js
button_find.js
button_history.js
button_paste.js
button_print.js
button_save.js

I suspect others, but I would comment out one or two that failed and push.

Here is a link to a log if that would be beneficial:
https://tbpl.mozilla.org/php/getParsedLog.php?id=38554907&tree=Try
(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) (PTO, back on May the 5th) from comment #3)
> As far as I see from the logs, the test timeouts somewhere between the
> info() and the assertion lines (the info message is displayed but the
> assertion is not):
> 
> info("Check print button existence and functionality");
> yield PanelUI.show();
> 
> let printButton = document.getElementById("print-button");
> ok(printButton, "Print button exists in Panel Menu");
> 
> I'm thinking that the panel menu doesn't show, but I don't know what the
> reason for that could be. I found a similar failure in the
> browser_947914_button_addons test (the first one run from the patch from bug
> 947914) and that makes me exclude the possibility of the timeout being
> caused by something going wrong in the previous run tests from that patch.
> 
> I also think that a similar failure but with PanelUI.hide(); is the cause of
> the failures from bug 1001755.
> 
> I can add more messages in the tests to help debugging, but I guess it will
> take some time until we get the results given it's something intermittent.
> 
> Gijs, what do you think?

I think intermittent test failures are really frigging annoying. I don't know how to fix these.

We've had these issues elsewhere (e.g. our attempts to figure out bug 976544).

The only thing I know to reliably cause this stuff to fail is window closures / mode changes (minimize, maximize, fullscreen) which tests don't wait for and which then happen in massive chunks in the middle of later tests, causing them to ... up.

This is unfortunate because all the work in bug 947914 is about opening these panels. If we can't rely on them opening, that's going to make landing any of the tests very, very hard.

I know there's work to make tests run per-directory. I would actually be tempted to suggest making bug 947914 block that work, and then try to land again.

Then again:

(In reply to Joel Maher (:jmaher) from comment #51)
> I was going to file a bug on this but was travelling.  When I updating my
> code base to sanity check before filing it was backed out.
> 
> In bug 992911 we are running tests as a single directory.  With that said, I
> see a lot of timeouts on linux (opt + debug), here are the tests that I saw
> timeout:
> button_addons.js
> button_copy.js
> button_cut.js
> button_find.js
> button_history.js
> button_paste.js
> button_print.js
> button_save.js
> 
> I suspect others, but I would comment out one or two that failed and push.
> 
> Here is a link to a log if that would be beneficial:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=38554907&tree=Try

This suggests something more problematic is happening. It might be interesting to try to bisect the directory (so create a try push based on bug 992911 with some of the previous tests skipped, and see if they make these tests stop failing so often).

It is also possible that we are missing something dumb. I'm going to request Jared to look at these tests to have a fresh pair of eyes look at them. :-)
Comment on attachment 8412537 [details] [diff] [review]
partial patch

Jared, can you give this a look to see if I missed something obvious which would explain all the linux failures?
Attachment #8412537 - Flags: review?(jaws)
I see 2 possible reasons :
- the panel animation doesn't work on Linux, which causes the tests to fail as panels don't show
- absence of clean up functions
Comment on attachment 8412537 [details] [diff] [review]
partial patch

Review of attachment 8412537 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/test/browser_947914_button_cut.js
@@ +6,5 @@
> +
> +add_task(function() {
> +  info("Check cut button existence and functionality");
> +
> +  var testText = "cut text test";

Why is this using var ?

@@ +13,5 @@
> +  yield PanelUI.show();
> +
> +  let cutButton = document.getElementById("cut-button");
> +  ok(cutButton, "Cut button exists in Panel Menu");
> +  ok(cutButton.getAttribute("disabled"), "Cut button is disabled");

hasAttribute() :)

FYI, a string always returns true. So the test might fail with !cutButton.hasAttribute("disabled") . Unless JS works differently in browser tests.
(In reply to Tim Nguyen [:ntim] from comment #55)
> Comment on attachment 8412537 [details] [diff] [review]
> partial patch
> 
> Review of attachment 8412537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/test/browser_947914_button_cut.js
> @@ +6,5 @@
> > +
> > +add_task(function() {
> > +  info("Check cut button existence and functionality");
> > +
> > +  var testText = "cut text test";
> 
> Why is this using var ?

var vs. let in tests is not a huge deal, especially if the function is quite short.

> @@ +13,5 @@
> > +  yield PanelUI.show();
> > +
> > +  let cutButton = document.getElementById("cut-button");
> > +  ok(cutButton, "Cut button exists in Panel Menu");
> > +  ok(cutButton.getAttribute("disabled"), "Cut button is disabled");
> 
> hasAttribute() :)
> 
> FYI, a string always returns true. So the test might fail with
> !cutButton.hasAttribute("disabled") . Unless JS works differently in browser
> tests.

An empty string is not truthy. var a = !!""; /* produces false */ a = !""; /* produces true */
Comment on attachment 8412537 [details] [diff] [review]
partial patch

Review of attachment 8412537 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see any obvious issues with the tests here.
Attachment #8412537 - Flags: review?(jaws)
Attached patch patch (obsolete) — Splinter Review
Added cleanup functions and some info messages (for later debugging if necessary)

remote:   https://tbpl.mozilla.org/?tree=Try&rev=ec31289efd8d
Attachment #8412537 - Attachment is obsolete: true
Attachment #8420100 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8420100 [details] [diff] [review]
patch

First of all, thanks for sticking to this. It might be the longest-running review session I've done, and if I were writing these, it'd definitely be the longest-running patching session I'd have done. I really appreciate your efforts. :-)

I looked at a couple of these, and I think it's a sensible idea to save which tab we added and to always clean it up when exiting, and to split off all the other cleanup tasks.

However, I'm clearing review for now because:

- you mix a lot of var and let. In browser JS, we pretty much exclusively use 'let' (and 'const') now. (I actually don't necessarily think this is a good idea, but that's not relevant for this review.) Please adjust your patch so as not to introduce new "var" declarations.
- you seem to have regressed some of the nits from my earlier reviews (such as using ok(foo == "") rather than is(foo, "") ).
- I'm noticing some extra spaces ("was  opened") and lacking spaces (if(gInPrintPreview))
- I'm not sure why you're now checking for the print preview toolbar rather than for gInPrintPreview after entering print preview. I don't think that change is correct.

Please adjust these and re-request review. Thanks!
Attachment #8420100 - Flags: review?(gijskruitbosch+bugs)
Attached patch updated patch (obsolete) — Splinter Review
Attachment #8420100 - Attachment is obsolete: true
Attachment #8421667 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8421667 [details] [diff] [review]
updated patch

Review of attachment 8421667 [details] [diff] [review]:
-----------------------------------------------------------------

I've got some nits, and some questions about cut/copy/paste, but otherwise, this seems sane. Best get a try run with a bunch of retriggers before landing, though...

::: browser/components/customizableui/test/browser_947914_button_copy.js
@@ +16,5 @@
> +  info("Menu panel was opened");
> +
> +  let copyButton = document.getElementById("copy-button");
> +  ok(copyButton, "Copy button exists in Panel Menu");
> +  is(copyButton.getAttribute("disabled"), "true", "Copy button is initially disabled");

Did this ever fail in those oranges we got? I am suspicious that this might not be the case if the URL bar was already focused, for instance...

::: browser/components/customizableui/test/browser_947914_button_cut.js
@@ +16,5 @@
> +  info("Menu panel was opened");
> +
> +  let cutButton = document.getElementById("cut-button");
> +  ok(cutButton, "Cut button exists in Panel Menu");
> +  ok(cutButton.getAttribute("disabled"), "Cut button is disabled");

Same here.

::: browser/components/customizableui/test/browser_947914_button_zoomIn.js
@@ +31,5 @@
> +});
> +
> +add_task(function asyncCleanup() {
> +    // reset zoom level
> +  ZoomManager.zoom = initialPageZoom;

Nit: line up the comment with the line below.

::: browser/components/customizableui/test/browser_947914_button_zoomOut.js
@@ +17,5 @@
> +  let zoomOutButton = document.getElementById("zoom-out-button");
> +  ok(zoomOutButton, "Zoom out button exists in Panel Menu");
> +
> +  zoomOutButton.click();
> +  let pageZoomLevel = Math.round(ZoomManager.zoom*100);

Nit: add spaces around the *

@@ +32,5 @@
> +});
> +
> +add_task(function asyncCleanup() {
> +    // reset zoom level
> +  ZoomManager.zoom = initialPageZoom;

More alignment

::: browser/components/customizableui/test/browser_947914_button_zoomReset.js
@@ +17,5 @@
> +  let zoomResetButton = document.getElementById("zoom-reset-button");
> +  ok(zoomResetButton, "Zoom reset button exists in Panel Menu");
> +
> +  zoomResetButton.click();
> +  let pageZoomLevel = parseInt(ZoomManager.zoom * 100);

Nit: Math.floor(ZoomManager.zoom * 100);

Also, these variable names seem to be the wrong way around. Maybe instead:

let expectedZoomLevel = 100;
let pageZoomLevel = Math.floor(ZoomManager.zoom * 100);
is(pageZoomLevel, expectedZoomLevel, "Page zoom should reset to 100");
let buttonZoomLevel = parseInt(zoomResetButton.getAttribute("label"), 10);
is(buttonZoomLevel, pageZoomLevel, "Button should display correct zoom level");

@@ +30,5 @@
> +});
> +
> +add_task(function asyncCleanup() {
> +    // reset zoom level
> +  ZoomManager.zoom = initialPageZoom;

Nit: more alignment
Attachment #8421667 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch v14 (obsolete) — Splinter Review
Resolved the requested updates. Carrying over the r+

(In reply to :Gijs Kruitbosch from comment #61)
> ::: browser/components/customizableui/test/browser_947914_button_copy.js
> @@ +16,5 @@
> > +  info("Menu panel was opened");
> > +
> > +  let copyButton = document.getElementById("copy-button");
> > +  ok(copyButton, "Copy button exists in Panel Menu");
> > +  is(copyButton.getAttribute("disabled"), "true", "Copy button is initially disabled");
> 
> Did this ever fail in those oranges we got? I am suspicious that this might
> not be the case if the URL bar was already focused, for instance...

I think it did fail at some moment. I added a gURLBar.focus(); just before opening the panel. (same for the cut test).

remote:   https://tbpl.mozilla.org/?tree=Try&rev=86963b215801
Attachment #8421667 - Attachment is obsolete: true
Attachment #8422403 - Flags: review+
Try runs: https://tbpl.mozilla.org/?tree=Try&rev=5ed5caafeb3c
Attachment #8422403 - Attachment is obsolete: true
Attachment #8496869 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8496869 [details] [diff] [review]
tests (skipped on Linux because of intermittent failures)

Review of attachment 8496869 [details] [diff] [review]:
-----------------------------------------------------------------

I reviewed this via interdiff, and it looks good apart from the following:

::: browser/components/customizableui/test/browser.ini
@@ +82,5 @@
> +skip-if = os == "linux" # Intermittent failures
> +[browser_947914_button_paste.js]
> +skip-if = os == "linux" # Intermittent failures
> +[browser_947914_button_preferences.js]
> +skip-if = os == "linux" # Intermittent failures

button_preferences.js wasn't part of the previous few patches for a long time. By now I've forgotten why, but I do know that the test isn't good as-is. It relies on in-content preferences, which only exist on Nightly, meaning the test will go orange once it hits aurora.

Please land this without the button_preferences.js test and associated browser.ini section, and file a followup for it once this has successfully landed. :-)
Attachment #8496869 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #65)
> button_preferences.js wasn't part of the previous few patches for a long
> time. By now I've forgotten why, but I do know that the test isn't good
> as-is. It relies on in-content preferences, which only exist on Nightly,
> meaning the test will go orange once it hits aurora.

Yeah, you're right about the preferences test. I was also wondering about that, but I missed that argument. I think at first it was removed because of the intermittent failures.
Removed the test from the patch.
Thanks!
Attachment #8496869 - Attachment is obsolete: true
Attachment #8497331 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/560e8a68cbf7
Keywords: checkin-needed
Whiteboard: [Australis:P-] → [Australis:P-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/560e8a68cbf7
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P-][fixed-in-fx-team] → [Australis:P-]
Target Milestone: --- → Firefox 35
See Also: → 1087303
You need to log in before you can comment on or make changes to this bug.