656295 - Split testSetToCurrentPage.js into two modules

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Alex Lakatos[:AlexLakatos]]

File: tests/functional/testPreferences/testSetToCurrentPage.js

This test module consists of two tests:
 * testSetHomePage() - https://litmus.mozilla.org/show_test.cgi?id=15430
 * testHomeButton() - https://litmus.mozilla.org/show_test.cgi?id=15497

These should be split into two separate test modules.  I recommend moving testHomeButton() to its own module testHomeButton.js.

Comment 1

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

Comment 2

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/13276953

Comment 3

[u415141]

Henrik Skupin changed story state to started in Pivotal Tracker

Comment 4

[u279076]

Comment on attachment 531634 [details] [diff] [review]
patch v1.0

>diff --git a/tests/functional/testPreferences/testHomeButton.js b/tests/functional/testPreferences/testHomeButton.js
>new file mode 100644
>--- /dev/null
>+++ b/tests/functional/testPreferences/testHomeButton.js
>@@ -0,0 +1,76 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Mozmill Test Code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Aakash Desai <[e-mail]>
>+ *   Henrik Skupin <[e-mail]>
>+ *   Aaron Train <[e-mail]>
>+ *   Alex Lakatos <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+// Include the required modules
>+var prefs = require("../../../lib/prefs");
>+var tabs = require("../../../lib/tabs");
>+var toolbars = require("../../../lib/toolbars");
>+
>+var setupModule = function() {
>+  controller = mozmill.getBrowserController();
>+  locationBar = new toolbars.locationBar(controller);
>+
>+  tabs.closeAllTabs(controller);
>+}
>+
>+var teardownModule = function(module) {
>+  prefs.preferences.clearUserPref("browser.startup.homepage");
>+}
>+
>+/**
>+ * Test the home button
>+ */
>+var testHomeButton = function()
>+{
>+  // Open another local page before going to the home page
>+  controller.open('www.google.com');
>+  controller.waitForPageLoad();

Can you update this test to use a local test page, like testSetToCurrentPage() does?

>+  // Go to the saved home page and verify it's the correct page
>+  controller.click(new elementslib.ID(controller.window.document, "home-button"));
>+  controller.waitForPageLoad();

Can you assign the element to its own variable before clicking on it?

Comment 5

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0

Used a local page and assigned homeButton before clicking it.

Comment 6

[u279076]

Comment on attachment 531875 [details] [diff] [review]
patch v2.0

>+var testHomeButton = function()
>+{

Nit: Function signature should use the following format:
function testFunction() {

>+  // Go to the saved home page and verify it's the correct page
>+  homeButton = new elementslib.ID(controller.window.document, "home-button");

Nit: Please use the 'var' keyword when declaring variables.

>+/**
>+ * Map test functions to litmus tests
>+ */
>+// testHomeButton.meta = {litmusids : [15497]};

I think this can be removed. Henrik, please advise.

>--- a/tests/functional/testPreferences/testSetToCurrentPage.js
>  * The Initial Developer of the Original Code is Mozilla Foundation.
>  * Portions created by the Initial Developer are Copyright (C) 2009
>  * the Initial Developer. All Rights Reserved.

Please update the year to 2011.

>   // Go to the saved home page and verify it's the correct page
>   controller.click(new elementslib.ID(controller.window.document, "home-button"));
>   controller.waitForPageLoad();

Sorry I missed this in the initial review, but please move the element declaration to its own line.

> /**
>  * Map test functions to litmus tests
>  */
>+// testSetHomePage.meta = {litmusids : [15430]};

Again, I'm not sure we need this anymore. Henrik, please advise.

Comment 7

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v3.0

Comment 8

[u279076]

Comment on attachment 532174 [details] [diff] [review]
patch v3.0

Moving this into Henrik's queue since I won't have time for this today.

Comment 9

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

Comment on attachment 532174 [details] [diff] [review]
patch v3.0

>+++ b/tests/functional/testPreferences/testHomeButton.js

We should move this test under testToolbars, because that's the place where the button is placed.

>+  // Verify location bar with the saved home page
>+  controller.assertValue(locationBar.urlbar, 'about:home');

We should not hard-code any URL. In a case like this we can modify the pref directly, and set the local test page as the home page. There is no need to use the preference dialog, also because it's covered by the other test.

Just one more note for testSetHomePage. In this test we really don't need multiple local pages. Just load 'about:blank' or call closeAllTabs() before you click the home button. Right now this test leaks a test to ensure the 2nd page has been really loaded. So if this step would fail, we get a wrong result for the test.

Comment 10

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.0

(In reply to comment #9)
> We should move this test under testToolbars, because that's the place where
> the button is placed.
Done

> We should not hard-code any URL. In a case like this we can modify the pref
> directly, and set the local test page as the home page. There is no need to
> use the preference dialog, also because it's covered by the other test.
Done, using setPref.

> Just one more note for testSetHomePage. In this test we really don't need
> multiple local pages. Just load 'about:blank' or call closeAllTabs() before
> you click the home button. Right now this test leaks a test to ensure the
> 2nd page has been really loaded. So if this step would fail, we get a wrong
> result for the test.
Done using closeAllTabs().

Comment 11

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

Comment on attachment 532894 [details] [diff] [review]
patch v4.0

>- * Portions created by the Initial Developer are Copyright (C) 2009
>+ * Portions created by the Initial Developer are Copyright (C) 2011

nit: Please don't change the date for existing tests. This year reflects the time the test has been added to the repository, but not any modification date.

> var setupModule = function() {
> var teardownModule = function(module) {

While updating the above line, can you simply also update those two lines so we make use of named functions?

>+  // Closing all tabs
>+  tabs.closeAllTabs(controller);

nit: This call should be clear enough which shouldn't require an additional comment.

>+++ b/tests/functional/testToolbar/testHomeButton.js

>+var setupModule = function() {
>+var teardownModule = function(module) {

nit: Same as above.

>+  prefs.preferences.setPref("browser.startup.homepage", LOCAL_TEST_PAGES[0]);  
>+  prefs.preferences.clearUserPref("browser.startup.homepage");

Please move the pref name to a constant.

>+function testHomeButton() {
>+  // Open another local page before going to the home page
>+  controller.open(LOCAL_TEST_PAGES[1]);
>+  controller.waitForPageLoad();

Same as for the other test module. We only need a single test page here. We close all tabs so we know that no other page is currently displayed.

Looks like we will be ready for check-in after the next round.

Comment 12

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v5.0

Comment 13

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v5.1

Comment 14

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

Comment on attachment 532945 [details] [diff] [review]
patch v5.1

In general it looks fine now and you will get a r+. Can you simply do me the following favor?

>+const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'layout/mozilla.html';
>+const BROWSER_HOMEPAGE = "browser.startup.homepage";

Please separate constants of different areas by a single empty line. That way we can easier check for their relationship. It applies to both tests.

>+function testHomeButton() {
>+
>+  // Go to the saved home page and verify it's the correct page

You can get rid of this empty line.

Once updated I will checkin the patch. Thanks Alex.

Comment 15

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v5.2

Comment 16

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/c02aa1c2a146 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/afac60fd0500 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/79ab5f8f1583 (beta)

Please update the Litmus tests for the new path and close the bug as fixed afterward. Thanks.

Comment 17

[Alex Lakatos[:AlexLakatos]]

Updated https://litmus.mozilla.org/show_test.cgi?id=15497
Updated https://litmus.mozilla.org/show_test.cgi?id=15430
Marking this RESOLVED FIXED

Comment 18

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

Comment on attachment 533211 [details] [diff] [review]
patch v5.2

Fixed missing review flag.

Comment 19

[u279076]

Verified fixed based on absence of failures in the daily reports.