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

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: whimboo, Assigned: daniela.p98911)

Tracking

unspecified

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed, firefox20 fixed, firefox21 fixed, firefox-esr10 fixed, firefox-esr17 fixed)

Details

(Whiteboard: s=121112 u=enhancement c=assertions p=1)

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Whiteboard: [mentor=whimboo][lang=js][good first bug] → s=121112 u=enhancement c=assertions p=1
(Assignee)

Updated

6 years ago
Assignee: nobody → dpetrovici
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
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.
(Reporter)

Comment 2

6 years ago
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 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.
Attachment #690909 - Flags: review?(hskupin)
Attachment #690909 - Flags: review?(dave.hunt)
Attachment #690909 - Flags: review?(andreea.matei)
Attachment #690909 - Flags: review-
(Assignee)

Comment 5

6 years ago
Created attachment 693482 [details] [diff] [review]
patch v1.1

Modified patch based on review
Attachment #690909 - Attachment is obsolete: true
Attachment #693482 - Flags: review?(hskupin)
Attachment #693482 - Flags: review?(dave.hunt)
Attachment #693482 - Flags: review?(andreea.matei)
(Reporter)

Comment 6

6 years ago
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
Attachment #693482 - Flags: review?(hskupin)
Attachment #693482 - Flags: review?(dave.hunt)
Attachment #693482 - Flags: review?(andreea.matei)
Attachment #693482 - Flags: review-
(Assignee)

Comment 7

6 years ago
Created attachment 696297 [details] [diff] [review]
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
Attachment #693482 - Attachment is obsolete: true
Attachment #696297 - Flags: review?(hskupin)
Attachment #696297 - Flags: review?(dave.hunt)
Attachment #696297 - Flags: review?(andreea.matei)
(Reporter)

Comment 8

6 years ago
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.
Attachment #696297 - Flags: review?(hskupin)
Attachment #696297 - Flags: review?(dave.hunt)
Attachment #696297 - Flags: review?(andreea.matei)
Attachment #696297 - Flags: review-
(Assignee)

Comment 9

6 years ago
Created attachment 697397 [details] [diff] [review]
patch v3.0

Changed the patch to use the node instead of the element per comment #6
Attachment #696297 - Attachment is obsolete: true
Attachment #697397 - Flags: review?(hskupin)
Attachment #697397 - Flags: review?(dave.hunt)
Attachment #697397 - Flags: review?(andreea.matei)
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?
Attachment #697397 - Flags: review?(hskupin)
Attachment #697397 - Flags: review?(dave.hunt)
Attachment #697397 - Flags: review?(andreea.matei)
Attachment #697397 - Flags: review-
(Reporter)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
Created attachment 697891 [details] [diff] [review]
patch v4.0

Used existing method exists() as per the discussion on IRC
Attachment #697397 - Attachment is obsolete: true
Attachment #697891 - Flags: review?(hskupin)
Attachment #697891 - Flags: review?(dave.hunt)
Attachment #697891 - Flags: review?(andreea.matei)
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.
Attachment #697891 - Flags: review?(hskupin)
Attachment #697891 - Flags: review?(dave.hunt)
Attachment #697891 - Flags: review?(andreea.matei)
Attachment #697891 - Flags: review-
(Assignee)

Comment 14

6 years ago
Created attachment 701026 [details] [diff] [review]
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
Attachment #697891 - Attachment is obsolete: true
Attachment #701026 - Flags: review?(hskupin)
Attachment #701026 - Flags: review?(dave.hunt)
Attachment #701026 - Flags: review?(andreea.matei)
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().
Attachment #701026 - Flags: review?(hskupin)
Attachment #701026 - Flags: review?(dave.hunt)
Attachment #701026 - Flags: review?(andreea.matei)
Attachment #701026 - Flags: review+
Keywords: checkin-needed
(Reporter)

Comment 16

6 years ago
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.
status-firefox-esr10: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox-esr17: --- → affected
Keywords: checkin-needed
(Assignee)

Comment 17

6 years ago
Created attachment 702233 [details] [diff] [review]
patch v4.2

Modified ./tests/functional/testPrivateBrowsing/testStartStopPBMode.js so that we can land this patch across branches
Attachment #701026 - Attachment is obsolete: true
Attachment #702233 - Flags: review?(hskupin)
Attachment #702233 - Flags: review?(dave.hunt)
Attachment #702233 - Flags: review?(andreea.matei)
(Reporter)

Comment 18

6 years ago
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)
Attachment #702233 - Flags: review?(hskupin)
Attachment #702233 - Flags: review?(dave.hunt)
Attachment #702233 - Flags: review?(andreea.matei)
Attachment #702233 - Flags: review+
Attachment #702233 - Flags: checkin+
(Reporter)

Updated

6 years ago
status-firefox21: affected → fixed
(Reporter)

Comment 20

6 years ago
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.
status-firefox20: affected → fixed
(Assignee)

Comment 21

6 years ago
Created attachment 703953 [details] [diff] [review]
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).
Attachment #703953 - Flags: review?(hskupin)
Attachment #703953 - Flags: review?(dave.hunt)
Attachment #703953 - Flags: review?(andreea.matei)
(Reporter)

Comment 23

6 years ago
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)
Attachment #703953 - Flags: review?(hskupin)
Attachment #703953 - Flags: review?(dave.hunt)
Attachment #703953 - Flags: review?(andreea.matei)
Attachment #703953 - Flags: review+
Attachment #703953 - Flags: checkin+
(Reporter)

Comment 24

6 years ago
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)
Attachment #703964 - Flags: review?(hskupin)
Attachment #703964 - Flags: review?(dave.hunt)
Attachment #703964 - Flags: review?(andreea.matei)
Attachment #703964 - Flags: review+
Attachment #703964 - Flags: checkin+
(Reporter)

Comment 25

6 years ago
This is done now! \o/
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox-esr10: affected → fixed
status-firefox18: affected → fixed
status-firefox19: affected → fixed
status-firefox-esr17: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.