The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

unspecified

Firefox Tracking Flags

(firefox16 fixed, firefox17 fixed, firefox18 fixed, firefox19 fixed, firefox-esr10 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 676600 [details] [diff] [review]
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.
Attachment #676600 - Flags: review?(hskupin)

Updated

4 years ago
Attachment #676600 - Attachment is patch: true
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.
Attachment #676600 - Flags: feedback+
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.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Summary: Update to Mozmill test for the Stop and Reload buttons → Update test for stop and reload buttons to test what it really should test
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'.
Attachment #676600 - Flags: review?(hskupin) → review-
(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. :)
Created attachment 678449 [details] [diff] [review]
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.
Attachment #676600 - Attachment is obsolete: true
Attachment #676600 - Flags: review- → review?(hskupin)
Attachment #678449 - Flags: review?(hskupin)
Attachment #676600 - Flags: review?(hskupin)
Attachment #676600 - Flags: review-
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."
Attachment #678449 - Flags: review?(hskupin) → review-
Created attachment 678723 [details] [diff] [review]
Updated patch for testStopReloadButtons.js

This addresses all comments on the last patch.
Attachment #678449 - Attachment is obsolete: true
Attachment #678723 - Flags: review?(hskupin)
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)
Attachment #678723 - Flags: review?(hskupin)
Attachment #678723 - Flags: review+
Attachment #678723 - Flags: checkin+
Bob, can you please check if the patch cleanly applies to the other branches? If not please attach an updated version of the patch.
status-firefox-esr10: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → fixed
I tested applying the patch to mozilla-aurora, mozilla-beta, mozilla-esr10 and mozilla-release and it applied cleanly to each branch.
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)
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox-esr10: affected → fixed
status-firefox16: affected → fixed
status-firefox17: affected → fixed
status-firefox18: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.