809431 - Get rid of controller.assertNode() and controller.assertNodeNotExist() calls in favor of assert.* methods

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

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

There are still a lot of controller.assertNode() and controller.assertNodeNotExist() calls in our code. To have more verbose output of our tests and especially failures we should all of them convert to make use of assert.* methods instead. This will also drive us forward in better using Mozmill 2.0 soon.

I don't think any of those instances should be an expect.*() call.

Comment 1

[Alex Lakatos[:AlexLakatos]]

https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L922

Looking over assertNodeNotExist code, I'm seeing it uses a try/catch block. Do we need a separate function in the assertions module, in order to replace it? cause using assert.ok(!element.getNode()) is going to throw an error, seeing as how the element does not exist.

Comment 2

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

Check how it works with the MozElement class in Mozmill 2. Probably we would need a helper method in our utils.js module for now.

Comment 3

[Daniela Petrovici]

Attachment: patch v1.0

Reports on all platforms:
Windows - XP SP3
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167fd2ea9
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167fd3088
Linux - Ubuntu x86
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167f72e05
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167f6db9c
MAC - 10.7.5 x86_64
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167fd34a4
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167fd3082

Comment 4

[Andreea Matei [:AndreeaMatei]]

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

>diff --git a/lib/utils.js b/lib/utils.js
>+function nodeExists(aElement) {
>+  try {
>+  	if (aElement.getNode())
>+  	  return true;
>+  }
>+  catch (e) {
>+  	return false;
>+  }
>+}
>+
The indentation is wrong here and we need spaces instead of tabs.
Also please move the function above removePermission() in order to keep it alphabetically sorted (also getElementStyle() should be moved up since we're here)
 
>diff --git a/tests/functional/testFormManager/testDisableFormManager.js b/tests/functional/testFormManager/testDisableFormManager.js
>-  controller.assertNodeNotExist(popDownAutoCompList);
>+  assert.ok(!utils.nodeExists(popDownAutoCompList), "Element has not been found");
>   controller.assertValue(firstName, FNAME.substring(0,2));
>-
>+  
Here are trailing whitespaces.

>diff --git a/tests/functional/testPreferences/testPreferredLanguage.js b/tests/functional/testPreferences/testPreferredLanguage.js
>+  
>+  var firstElemPl = new elementslib.ID(controller.tabs.activeTab, "Zaloguj");
>+  var secondElemPl = new elementslib.ID(controller.tabs.activeTab, "Dokumenty");
>+  var thirdElemPl = new elementslib.ID(controller.tabs.activeTab, "Szukanie zaawansowane");
>+  

Trailing whitespaces, but beside that, this test is modified in bug 812435 where we remove completely the use of Google page.

>diff --git a/tests/functional/testPrivateBrowsing/testCloseWindow.js b/tests/functional/testPrivateBrowsing/testCloseWindow.js
>-  {url: LOCAL_TEST_FOLDER + 'layout/mozilla.html', name: 'community'},
>-  {url: LOCAL_TEST_FOLDER + 'layout/mozilla_mission.html', name: 'mission'}
>+  {url : LOCAL_TEST_FOLDER + 'layout/mozilla.html', name : 'community'},
>+  {url : LOCAL_TEST_FOLDER + 'layout/mozilla_mission.html', name : 'mission'}
> ];
Why the extra spaces here?

>   // Open local pages in separate tabs and wait for each to finish loading
>   LOCAL_TEST_PAGES.forEach(function (page) {
>-    controller.open(page.url);
>-    controller.waitForPageLoad();
>+	controller.open(page.url);
>+	controller.waitForPageLoad();
> 
>-    var elem = new elementslib.Name(controller.tabs.activeTab, page.name);
>-    controller.assertNode(elem);
>+	var elem = new elementslib.Name(controller.tabs.activeTab, page.name);
>+	assert.ok(elem, "Element has been found");
> 
>-    tabBrowser.openTab();
>+	tabBrowser.openTab();
>   });

Indentation is wrong here, not sure why are you touching those lines beside the ones related to controller.assertNode().

>   // One single window will be opened in PB mode which has to be closed now
>   var cmdKey = utils.getEntity(tabBrowser.getDtds(), "closeCmd.key");
>-  controller.keypress(null, cmdKey, {accelKey: true});
>+  controller.keypress(null, cmdKey, {accelKey : true});

Please remove the extra space.

>-  mozmill.utils.waitFor(function () {
>-    return mozmill.utils.getWindows().length === (windowCount - 1);
>+  mozmill.utils.waitFor(function() {
>+	return mozmill.utils.getWindows().length === (windowCount - 1);
>   }, "The browser window has been closed");

Why have you removed the space between function and '(' ?

>-    tabBrowser.selectedIndex = i;
>-    controller.waitForPageLoad(controller.tabs.activeTab);
>+	tabBrowser.selectedIndex = i;
>+	controller.waitForPageLoad(controller.tabs.activeTab);

Indentations issues, there's no need to touch this code here.

>-    var elem = new elementslib.Name(controller.tabs.activeTab,
>-                                    LOCAL_TEST_PAGES[i].name);
>-    controller.assertNode(elem);
>+	var elem = new elementslib.Name(controller.tabs.activeTab, LOCAL_TEST_PAGES[i].name);
>+	assert.ok(elem, "Element has been found");
>   }
> }

Indentation here as well.

>diff --git a/tests/functional/testSecurity/testSafeBrowsingNotificationBar.js b/tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
>-const DOMAIN_NAME = "www.mozilla.org";
>-const WARNING_PAGES_URLS = ['http://' + DOMAIN_NAME + '/firefox/its-a-trap.html',
>-                            'http://' + DOMAIN_NAME + '/firefox/its-an-attack.html'];
>-

>-  for (var i = 0; i < WARNING_PAGES_URLS.length; i++ ) {
>+  var badSites = ['http://www.mozilla.org/firefox/its-a-trap.html',
>+                  'http://www.mozilla.org/firefox/its-an-attack.html'];
>+
>+  for (var i = 0; i < badSites.length; i++ ) {
>     // Go to one of mozilla's phishing protection test pages
>-    controller.open(WARNING_PAGES_URLS[i]);
>+    controller.open(badSites[i]);
>     controller.waitForPageLoad();
> 
>     // Wait for the ignoreWarning button to be loaded onto the page and then click on the button
>-    checkIgnoreWarningButton(WARNING_PAGES_URLS[i]);
>-    checkNoPhishingButton(WARNING_PAGES_URLS[i]);
>+    checkIgnoreWarningButton(badSites[i]);
>+    checkNoPhishingButton(badSites[i]);
> 
>     // Go back to the notification bar
>-    controller.open(WARNING_PAGES_URLS[i]);
>+    controller.open(badSites[i]);
>     controller.waitForPageLoad();
>-    checkIgnoreWarningButton(WARNING_PAGES_URLS[i]);
>+    checkIgnoreWarningButton(badSites[i]);
> 
>     // Test the get me out of here button
>     checkGetMeOutOfHereButton();
> 
>     // Go back to the notification bar
>-    controller.open(WARNING_PAGES_URLS[i]);
>+    controller.open(badSites[i]);
>     controller.waitForPageLoad();
>-    checkIgnoreWarningButton(WARNING_PAGES_URLS[i]);
>+    checkIgnoreWarningButton(badSites[i]);


Why all these changes? What was wrong with WARNING_PAGES_URLS const?

>-  controller.assertNodeNotExist(ignoreWarningButton);
>-  controller.assertNode(new elementslib.ID(controller.tabs.activeTab, "main-feature"));
>+  assert.ok(!utils.nodeExists(ignoreWarningButton), "Warning button isn't visible");

Please use "is not" which is more official, instead of "isn't".

>   controller.sleep(1000);
>-  controller.assertNodeNotExist(button);
>+  assert.ok(!utils.nodeExists(button), "Close button hasn't been found");
> }

Same as above.

>diff --git a/tests/functional/testSecurity/testSafeBrowsingWarningPages.js b/tests/functional/testSecurity/testSafeBrowsingWarningPages.js

>-const WARNING_PAGES_URLS = ['http://' + DOMAIN_NAME + '/firefox/its-a-trap.html',
>-                            'http://' + DOMAIN_NAME + '/firefox/its-an-attack.html'];

> var testWarningPages = function() {
>-  for (var i = 0; i < WARNING_PAGES_URLS.length; i++ ) {
>+  var urls = ['http://' + DOMAIN_NAME + '/firefox/its-a-trap.html',
>+              'http://' + DOMAIN_NAME + '/firefox/its-an-attack.html'];
>+
>+  for (var i = 0; i < urls.length; i++ ) {
>     // Open one of the mozilla phishing protection test pages
>-    controller.open(WARNING_PAGES_URLS[i]);
>+    controller.open(urls[i]);
>     controller.waitForPageLoad();

>     // Go back to the warning page
>-    controller.open(WARNING_PAGES_URLS[i]);
>+    controller.open(urls[i]);
>     controller.waitForPageLoad();
> 
>     // Test the reportButton
>-    checkReportButton(i, WARNING_PAGES_URLS[i]);
>+    checkReportButton(i, urls[i]);
> 
>     // Go back to the warning page
>-    controller.open(WARNING_PAGES_URLS[i]);
>+    controller.open(urls[i]);
>     controller.waitForPageLoad();
> 
>     // Test the ignoreWarning button
>-    checkIgnoreWarningButton(WARNING_PAGES_URLS[i]);
>+    checkIgnoreWarningButton(urls[i]);
>   }
> }

Same as for the test above, why did you felt the need to change this?
 
>-  controller.assertNodeNotExist(ignoreWarningButton);
>-  controller.assertNode(new elementslib.ID(controller.tabs.activeTab, "main-feature"));
>+  assert.ok(!utils.nodeExists(ignoreWarningButton), "Element hasn't been found");

Please use the full words - "has not been".

>diff --git a/tests/functional/testSecurity/testUnknownIssuer.js b/tests/functional/testSecurity/testUnknownIssuer.js

>-  // Verify "Get Me Out Of Here!" button appears
>-  controller.assertNode(new elementslib.ID(controller.tabs.activeTab, "getMeOutOfHereButton"));
>+  // Verify "Get Me Out Of Here!" button appears 

You have trailing whitespace here.

>+  var exceptionButton = new elementslib.ID(controller.tabs.activeTab, "exceptionDialogButton");
>+  assert.ok(exceptionButton, "'Add Exception' button appears"); 

Trailing whitespace here as well.

> setupModule.__force_skip__ = "Bug 763159 - Test failure 'secure.mur.at == erle.mur.at'" +
>-                             " in testSecurity/testUnknownIssuer.js";
>+                             "in testSecurity/testUnknownIssuer.js";
You have removed the space that makes the message readable.


>diff --git a/tests/functional/testToolbar/testStopReloadButtons.js b/tests/functional/testToolbar/testStopReloadButtons.js
>-  controller.assertNodeNotExist(footer);
>+  assert.ok(!utils.nodeExists(footer), "Element hasn't been found");

Please use the full words - "has not been".

Overall I would appreciate more meaningful messages for the assert calls, where possible, besides the "Element has been found" that doesn't give details about which is that element.
Thanks.

Comment 5

[Daniela Petrovici]

Attachment: patch v1.1

Modified patch based on review

Comment 6

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

Comment on attachment 693482 [details] [diff] [review]
patch v1.1

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

::: tests/functional/testAwesomeBar/testGoButton.js
@@ +50,5 @@
>    expect.ok(!utils.isDisplayed(controller, goButton), "Go button is hidden");
>  
>    // Check if an element with an id of 'organization' exists
>    var pageElement = new elementslib.ID(controller.tabs.activeTab, "organization");
> +  assert.ok(pageElement, "'Organization' element has been found");

All those changes are incorrect because it will not exactly replace the assertNode() method of the MozMillController class. In your case all those tests will pass every time. Please make sure to consolidate the mozmill source code in the future first:

https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L910

Comment 7

[Daniela Petrovici]

Attachment: patch v2.0

Changed patch to have assertNode and assertNodeNotExist in assertions library. The two methods are created now based on the Mozmill source code.
I have also modified a test in functional/testPrivateBrowsing, but this will need to be changed in case bug 818456 is fixed first

Comment 8

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

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

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

No, we do not want an assertNode() or assertNodeNotExist() method in the assertions module. And that's also not what I have pointed out in my last review comment. If something is unclear please ask before implementing the code. Reason for the above statement is that such methods are not compliant to the common JS module specification we mimic in Mozmill. That's why also the summary of this bug.

Comment 9

[Daniela Petrovici]

Attachment: patch v3.0

Changed the patch to use the node instead of the element per comment #6

Comment 10

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

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

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

::: lib/tests/testTabView.js
@@ +37,5 @@
>        value: element.value,
>        parent: element.parent
>      });
>  
> +    assert.ok(node, "Element has been found");

Why aren't we using utils.nodeExists here and in other places in this patch?

Comment 11

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

(In reply to Dave Hunt (:davehunt) from comment #10)
> > +    assert.ok(node, "Element has been found");
> 
> Why aren't we using utils.nodeExists here and in other places in this patch?

Huh? Why does this patch introduce another kind of helper method in utils? As we talked on IRC you should directly use %element%.getNode() for all the checks you are doing.

Comment 12

[Daniela Petrovici]

Attachment: patch v4.0

Used existing method exists() as per the discussion on IRC

Comment 13

[Andreea Matei [:AndreeaMatei]]

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

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

If using exists() is what you agreed on, there are a few things left and an update to the patch cause this does not apply cleanly anymore.

::: lib/tests/testTabView.js
@@ +37,5 @@
>        value: element.value,
>        parent: element.parent
>      });
>  
> +    assert.ok(node.exists(), "Element " + node + " exists");

'node' will show "[object Object]" that is not very helpful, so we can leave a generic message.

::: tests/functional/testFormManager/testDisableFormManager.js
@@ +62,5 @@
>                                '/{"class":"autocomplete-treebody"}'
>    );
>  
> +  assert.ok(!popDownAutoCompList.exists(),
> +           "Form completion element has not been found");

Message needs to be shifted one space to the right.

@@ +69,5 @@
>    lastName = new elementslib.ID(controller.tabs.activeTab, "ship_lname");
>    controller.type(lastName, LNAME.substring(0,2));
>    controller.sleep(TIMEOUT);
> +  assert.ok(!popDownAutoCompList.exists(),
> +           "Form completion element has not been found");

Same here.

Comment 14

[Daniela Petrovici]

Attachment: patch v4.1

Changes done per review. For the error message containing "node" I have changed back to the correct message (that I have changed by mistake in the previous patch).

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/bddfde7ab90491e4945eb7577c116cdf
http://mozmill-crowd.blargon7.com/#/functional/report/bddfde7ab90491e4945eb7577c116f66

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 701026 [details] [diff] [review]
patch v4.1

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

Looks good to me now, unless Henrik or Dave think otherwise about using node.exist().

Comment 16

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

We cannot land this because we still have entities of those methods left:

./tests/functional/testPrivateBrowsing/testStartStopPBMode.js:    controller.assertNode(elem);
./tests/functional/testPrivateBrowsing/testStartStopPBMode.js:    controller.assertNode(elem);

Given that we want to have the same patch across branches, we cannot cover that in the private browsing patch. Please make this quick change so we can land this here.

Comment 17

[Daniela Petrovici]

Attachment: patch v4.2

Modified ./tests/functional/testPrivateBrowsing/testStartStopPBMode.js so that we can land this patch across branches

Comment 18

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

Comment on attachment 702233 [details] [diff] [review]
patch v4.2

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

http://hg.mozilla.org/qa/mozmill-tests/rev/76e2da4a8e88 (default)

Comment 19

[Daniela Petrovici]

The patch for default clearly applies for Aurora branch. Reports are:
http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d33da15a
http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d33dae4a

Comment 20

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

http://hg.mozilla.org/qa/mozmill-tests/rev/3dd0aeed5457 (aurora)

For the other branches please report back all at once. That will be less invasive with the number of comments. Thanks.

Comment 21

[Daniela Petrovici]

Attachment: patch v1.0 for Beta, Release and ESR17 branches

Patch for Beta branch that was created on top of the patch for bug 793092 as it was discussed on IRC. 

This patch clearly applies for Release and ESR17 branches (on top of 793092).

Comment 22

[Daniela Petrovici]

Attachment: patch v1.0 for ESR branch

Beta:
http://mozmill-crowd.blargon7.com/#/functional/report/5dfc8c8f9af0c35c0952dab5300575a0
http://mozmill-crowd.blargon7.com/#/functional/report/5dfc8c8f9af0c35c0952dab530056ede
Release:
http://mozmill-crowd.blargon7.com/#/functional/report/5dfc8c8f9af0c35c0952dab53005e179
http://mozmill-crowd.blargon7.com/#/functional/report/5dfc8c8f9af0c35c0952dab53005a453
ESR17:
http://mozmill-crowd.blargon7.com/#/functional/report/5dfc8c8f9af0c35c0952dab53005fb66
http://mozmill-crowd.blargon7.com/#/functional/report/5dfc8c8f9af0c35c0952dab530060614
ESR10:
http://mozmill-crowd.blargon7.com/#/functional/report/5dfc8c8f9af0c35c0952dab53008bc59
http://mozmill-crowd.blargon7.com/#/functional/report/5dfc8c8f9af0c35c0952dab53006189b

Comment 23

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

Comment on attachment 703953 [details] [diff] [review]
patch v1.0 for Beta, Release and ESR17 branches

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

http://hg.mozilla.org/qa/mozmill-tests/rev/03a7f6805077 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/d5728363c705 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/94b2ead30a88 (esr17)

Comment 24

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

Comment on attachment 703964 [details] [diff] [review]
patch v1.0 for ESR branch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/2a25e19ac6b7 (esr10)

Comment 25

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

This is done now! \o/