806952 - Update test for stop and reload buttons to test what it really should test

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Bob Silverberg [:bsilverberg]]

Attachment: Patch for testStopReloadButtons.js

This is an update to http://hg.mozilla.org/qa/mozmill-tests/file/869473e61519/tests/functional/testToolbar/testStopReloadButtons.js which davehunt and I were working on. It also includes additions to http://hg.mozilla.org/qa/mozmill-tests/file/869473e61519/lib/tests/testToolbar.js and http://hg.mozilla.org/qa/mozmill-tests/file/869473e61519/lib/toolbars.js.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 676600 [details] [diff] [review]
Patch for testStopReloadButtons.js

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

Looks good to me, and passes locally. When attaching patches you should check the 'patch' option so the diff and review can be activated in Bugzilla. Also, see https://developer.mozilla.org/en-US/docs/Mozmill_Tests#The_review_process for details on the review process and suitable commit messages.

Comment 2

[Bob Silverberg [:bsilverberg]]

Thanks davehunt. I just used the hg diff command to create my patch and wasn't sure how to indicate a commit message. Do I need to use an hg queue to be able to add a commit message? I will look into hg queues before submitting my next patch for an issue.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 676600 [details] [diff] [review]
Patch for testStopReloadButtons.js

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

Overall the patch looks great. I have a couple of things I would like to see fixed. So the next iteration is most likely the final one. Thanks for spotting this Bob!

::: lib/tests/testToolbar.js
@@ +15,5 @@
>  
>  var testLocationBarAPI = function() {
>    // Test access to available elements
>    var input = locationBar.getElement({type: "urlbar_input"});
> +  expect.equal(input.getNode().localName, "input");

Equally to the reload button we should better check for something which uniquely identifies the location bar. The search bar is also an input field. Mind updating it together with this patch?

@@ +20,3 @@
>  
>    var contextMenu = locationBar.getElement({type: "contextMenu"});
> +  expect.equal(contextMenu.getNode().localName, "menupopup");

Does this context menu have an unique name we could check instead?

@@ +23,3 @@
>  
>    var contextMenuEntry = locationBar.getElement({type: "contextMenu_entry", subtype: "paste"});
> +  expect.equal(contextMenuEntry.getNode().localName, "menuitem");

Mind to checking the command instead?

@@ +25,5 @@
> +  expect.equal(contextMenuEntry.getNode().localName, "menuitem");
> +
> +  var reloadButton = locationBar.getElement({type: "reloadButton"});
> +  expect.equal(reloadButton.getNode().localName, "toolbarbutton");
> +  expect.equal(reloadButton.getNode().command, "Browser:ReloadOrDuplicate");

Good call with the command test. I would kill the check for the localName.

::: tests/functional/testToolbar/testStopReloadButtons.js
@@ +21,5 @@
> +
> +  /**
> +   * Set the pageUnloaded variable to true when page unloads
> +   */
> +  function onUnload() {

We only give top-level functions a jsdoc string. Please remove it completely.

@@ +24,5 @@
> +   */
> +  function onUnload() {
> +    pageUnloaded = true;
> +    controller.tabs.activeTab.defaultView.removeEventListener(
> +      "unload", onUnload, false

I would say lets create a local variable right at the beginning of the testStopAndReload test function for accessing any kind of property/method on 'controller.tabs.activeTab.defaultView'.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Bob Silverberg from comment #2)
> Thanks davehunt. I just used the hg diff command to create my patch and
> wasn't sure how to indicate a commit message. Do I need to use an hg queue
> to be able to add a commit message? I will look into hg queues before
> submitting my next patch for an issue.

It's definitely worthwhile reading up on mercurial queues. If you have any questions, just ask on IRC. :)

Comment 5

[Bob Silverberg [:bsilverberg]]

Attachment: Updated patch for testStopReloadButtons.js

This patch addresses the comments above, except for the change to the test for contextMenu as I was unable to find a unique identifier to use in place of localName.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 678449 [details] [diff] [review]
Updated patch for testStopReloadButtons.js

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

Not quite ready yet but there are really only some minor things left. Otherwise it looks fine!

::: tests/functional/testToolbar/testStopReloadButtons.js
@@ +17,5 @@
>   */
>  var testStopAndReload = function() {
> +
> +  var pageUnloaded = false;
> +  var defaultView = controller.tabs.activeTab.defaultView;

nit: Please call it contentWindow, which is more descriptive as defaultView.

@@ +21,5 @@
> +  var defaultView = controller.tabs.activeTab.defaultView;
> +
> +  function onUnload() {
> +    pageUnloaded = true;
> +      defaultView.removeEventListener("unload", onUnload, false);

nit: indentation issue. Also please switch both lines. Removing the event listener should always be in the first line.

@@ +29,5 @@
>    controller.open("about:blank");
>    controller.waitForPageLoad();
>  
> +  // Add event handler to the current page so we can wait for it to unload
> +    defaultView.addEventListener("unload", onUnload, false);

nit: indendation issue again.

@@ +35,3 @@
>    // Go to the URL and start loading for some milliseconds
>    controller.open(TEST_PAGE);
> +  controller.waitFor(function() {

Please add a blank between function and '('. Reason is that it's a noname function.

@@ +35,5 @@
>    // Go to the URL and start loading for some milliseconds
>    controller.open(TEST_PAGE);
> +  controller.waitFor(function() {
> +    return pageUnloaded;
> +  }, "Expected the about:blank page to be unloaded.");

To be consistent with our other waitFor() messages, we should update the message to "'about:blank' page has been unloaded."

Comment 7

[Bob Silverberg [:bsilverberg]]

Attachment: Updated patch for testStopReloadButtons.js

This addresses all comments on the last patch.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 678723 [details] [diff] [review]
Updated patch for testStopReloadButtons.js

Love it. Great work Bob! 

For now landed on default. If there is no regression we will backport the patch to older branches.

http://hg.mozilla.org/qa/mozmill-tests/rev/2f3c7d96c72e (default)

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Bob, can you please check if the patch cleanly applies to the other branches? If not please attach an updated version of the patch.

Comment 10

[Bob Silverberg [:bsilverberg]]

I tested applying the patch to mozilla-aurora, mozilla-beta, mozilla-esr10 and mozilla-release and it applied cleanly to each branch.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Backported to other branches:
http://hg.mozilla.org/qa/mozmill-tests/rev/f8ef5479cd04 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/bc479940f12f (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/a658d62c823c (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/94c5ea069ab9 (esr10)