525189 - [test] Update assertJS calls to match new API

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

In Bug 515072 we have updated the param set of the assertJS function. We will have to update all our assertJS calls to get more information. Simply tests should use a "throw new Error("...")" instead.

Comment 1

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

Tobias has been started to work on this. Moving over to him. Can you also please check for existing throw statements inside of tests and convert them to assertJS calls too? That would be great. Thanks!

Comment 2

[Tobias (:Tobbi) Markus]

Attachment: Part I: updated existing assertJS calls to match new API

I have finished and double-checked every test I modified! I hope it works for you. 

I'll add the throw{} ---> assertJS conversion in another patch.

Comment 3

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

Comment on attachment 430961 [details] [diff] [review]
Part I: updated existing assertJS calls to match new API

That looks great and it is a really useful patch. I have only some small nits:

Can you please shorten long lines and more the second parameter definition to its own line? It's mostly helpful when you have to specify an object. We try to not extend the 80 column restriction we normally have.

>--- a/firefox/testCookies/testRemoveCookie.js
>+  cmController.assertJS("subject.list.view.rowCount == subject.numberCookies - 1", {list: cookiesList, numberCookies: origNumCookies});

Do the calculation outside of the expression string. So we can keep the output as clean as possible.

>+++ b/firefox/testPreferences/testPaneRetention.js
>-  controller.assertJS('panePrivacy' == PrefsAPI.preferencesDialog.getPane(controller));
>+  controller.assertJS("'panePrivacy' == subject", PrefsAPI.preferencesDialog.getPane(controller));

Please use 'subject' always as the left term. Also please only use 'PrefsAPI.preferencesDialog' as parameter and let the function be called from inside the expression. 

>--- a/firefox/testSearch/testAddMozSearchProvider.js
>-  controller.assertJS(title == confirmTitle);
>+  controller.assertJS("subject.windowTitle == subject.confirmTitleProp", {windowTitle: title, confirmTitleProp: confirmTitle});

confirmTitleProp will not be the best option here. Can you please use something more descriptive?

>+++ b/firefox/testSearch/testGetMoreSearchEngines.js
>-  controller.assertJS(title == confirmTitle);
>+  controller.assertJS("subject.windowTitle == subject.confirmTitleProp", {windowTitle: title, confirmTitleProp: confirmTitle});

Same here.

>+++ b/firefox/testSearch/testOpenSearchAutodiscovery.js
>+  controller.assertJS("subject.getNode().getAttribute('addengines') == 'true'", engineButton);

getNode() should always be outside of the expression.

>+++ b/firefox/testSecurity/testEncryptedPageWarning.js
>-  controller.assertJS(modalWarningShown == true);
>+  controller.assertJS("subject == true", modalWarningShown);

Please wrap it in an object. Otherwise no-one knows what we are trying to check here. 

>+++ b/firefox/testSecurity/testSecurityNotification.js
>+  controller.assertJS("subject.getNode().textContent.indexOf('ssl_error_bad_cert_domain') != -1", text);

Please move this getNode call too.

>+++ b/firefox/testSecurity/testSubmitUnencryptedInfoWarning.js
>-  controller.assertJS(modalWarningShown == true);
>+  controller.assertJS("subject == true", modalWarningShown);

See above.

>+++ b/shared-modules/testSearchAPI.js
>-    this._controller.assertJS(searchInput.getNode() == activeElement);
>+    this._controller.assertJS("subject.input.getNode() == subject.element", {input: searchInput, element: activeElement});

See above for getNode and give it a better name.

Comment 4

[Tobias (:Tobbi) Markus]

Attachment: Part I v2 [checked-in]

Followup patch addressing the comments. I hope all is well now?

Comment 5

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

Landed on default branch as:
http://hg.mozilla.org/qa/mozmill-tests/rev/33a8d34d7a88

We need an additional fix for mozilla1.9.1:

atching file firefox/restartTests/testThemeInstallUninstall/test2.js
Hunk #1 FAILED at 88
1 out of 1 hunks FAILED -- saving rejects to file firefox/restartTests/testThemeInstallUninstall/test2.js.rej
patching file firefox/testInstallation/testBreakpadInstalled.js
Hunk #1 FAILED at 66
1 out of 1 hunks FAILED -- saving rejects to file firefox/testInstallation/testBreakpadInstalled.js.rej
patching file firefox/testSecurity/testSecurityNotification.js
Hunk #2 FAILED at 96
1 out of 2 hunks FAILED -- saving rejects to file firefox/testSecurity/testSecurityNotification.js.rej
abort: patch failed to apply

Comment 6

[Tobias (:Tobbi) Markus]

Attachment: v.1: Updating assertJS calls for mozilla1.9.1

This is v.1 for mozilla1.9.1 branch

Comment 7

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

Comment on attachment 432462 [details] [diff] [review]
v.1: Updating assertJS calls for mozilla1.9.1

Looks great. Only two nits here:

># Branch mozilla1.9.1
># Node ID 72e86ea0a23ade07c07ae474888b714000baa7d3
># Parent  87ff0a97a1c226734df84cb51d0ef0ca5653009b
>Update assertJS calls to match new API

Can you use a string like "Bug xxx - %summary%, r=hskupin"?

>-  controller.assertJS(execFile.exists() == true);
>+  controller.assertJS("subject.exists() == true", execFile);

Can you wrap that in another object please so we will have "subject.file.exists()..."

>     // If it is a valid key check the value for validity
>     if (states[key] != undefined) {
>-      controller.assertJS(states[key] == value);
>+      controller.assertJS("subject.currentState == subject.currentValue",
>+	                       {currentState: states[key], currentValue: value});

While reading the patch it's hard to understand what currentState and currentValue should tell us. We need something more descriptive here.

With those updated r=me.

Comment 8

[Tobias (:Tobbi) Markus]

Attachment: Part I v2 (1.9.1) [checked-in]

Okay, I think I fixed everything you said now. 

In testBreakpadInstalled.js I changed the following:
-      controller.assertJS(states[key] == value);
+      controller.assertJS("subject.iniState == subject.iniValue",
+	                       {iniState: states[key], iniValue: value});

I hope the variable naming is okay for you here? (I know I'm not good in inventing variable descriptors).

Comment 9

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

Comment on attachment 432793 [details] [diff] [review]
Part I v2 (1.9.1) [checked-in]

Lets take it. r=me

Comment 10

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

Patch I for 1.9.1 landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/a12ffba5c066

I'll leave this bug open for the remaining throw -> assertJS work.

Comment 11

[Tobias (:Tobbi) Markus]

Attachment: v.1: Converting throw to assertJS statements, branch: 1.9.2

Comment 12

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

Comment on attachment 433793 [details] [diff] [review]
v.1: Converting throw to assertJS statements, branch: 1.9.2

Wow, that's a really huge patch! I went through everything by hand and hope that I have catched everything. See my comments inline.

>+++ b/firefox/restartTests/testDefaultBookmarks/test1.js
>     // Check if the child is a Livemark
>-    if (!ls.isLivemark(child.itemId)) {
>-      throw "Latest Headlines toolbar button is not a Livemark";
>-    }
>+	controller.assertJS("subject.livemarkService.isLivemark(subject.bookmark.itemId)",
>+	                    {livemarkService: ls, bookmark: child});

Can you create an object from the return value of the function? So we do not have to pass in two different parameter. Just name it isLivemark. 

>+++ b/firefox/restartTests/testExtensionInstallUninstall/test1.js
> 
>   // There should be listed only one extension
>-  if (itemElem.childNodes.length != 1) {
>-    throw "Expected one extension for installation";
>-  }
>+  Controller.assertJS("subject.extList.length == 1", {extList: itemElem.childNodes});

Please use extensions instead of extList.

>+  Controller.assertJS("subject.extList[0].name == controller.extName",
>+                      {extList: itemElem.childNodes, extName: persisted.extensionName});

Same here please. Also check the other tests under this folder.

>   // Will the extension be installed from https://addons.mozilla.org/?
>-  if (itemElem.childNodes[0].url.indexOf("https://addons.mozilla.org/") == -1) {
>-    throw "Extension location doesn't contain https://addons.mozilla.org/";
>-  }
>+  Controller.assertJS("subject.extList[0].url.indexOf('https://addons.mozilla.org/') != -1", 
>+                      {extList: itemElem.childNodes});

Simplify this too by using subject.isExtensionFromAMO or similar.

>+++ b/firefox/restartTests/testExtensionInstallUninstall/test2.js
>   var window = mozmill.wm.getMostRecentWindow('Extension:Manager');
>-  if (!window)
>-    throw new Error("Addons Manager has not been opened automatically after the restart");
>+  controller.assertJS("subject.extManagerWindow != null", {extManagerWindow: window});

Can we get rid of window?

>+++ b/firefox/restartTests/testThemeInstallUninstall/test1.js
>+  controller.assertJS("subject.themesList.length == 1",
>+                      {themesList: itemElem.childNodes});

Please use themes.

>-  if (!update.allowed) {
>-    throw new Error("User does not have permissions to run the software update.");
>-  }
>+  controller.assertJS("subject.allowed == true", update);

Can you make it an object?

>+++ b/firefox/softwareUpdate/testDirectUpdate/test2.js
>     // If a major update is offered we shouldn't fail
>-    if (update.updateType == persisted.type)
>-      throw new Error("Another " + persisted.type + " update has been offered.");
>+	controller.assertJS("subject.updateType != subject.persistedUpdateType",
>+	                    {updateType: update.updateType, persistedUpdateType: persisted.type});

Please use newUpdateType != lastUpdateType.

>+++ b/firefox/softwareUpdate/testFallbackUpdate/test1.js
>-  if (!update.allowed) {
>-    throw new Error("User does not have permissions to run the software update.");
>-  }
>+  controller.assertJS("subject.softwareUpdate.allowed == true", {softwareUpdate: update});

Here it is. :) That's the way for the direct update too. You can also just use update.

>     // If a major update is offered we shouldn't fail
>-    if (update.updateType == persisted.type)
>-      throw new Error("Another " + persisted.type + " update has been offered.");
>+    controller.assertJS("subject.updateType != subject.persistedUpdateType",
>+	                    {updateType: update.updateType, persistedUpdateType: persisted.type});

See above.

>+++ b/firefox/testBookmarks/testAddBookmarkToMenu.js
>   // Fail if the URI is already bookmarked
>-  if (PlacesAPI.bookmarksService.isBookmarked(uri))
>-    throw "Failed: " + uri.spec + " is already bookmarked.";
>+  controller.assertJS("subject.bmksService.isBookmarked(subject.URI) == false",
>+                      {bmksService: PlacesAPI.bookmarksService, URI: uri});

Please move the function call out of the expression.

>-  if (!PlacesAPI.isBookmarkInFolder(uri, PlacesAPI.bookmarksService.bookmarksMenuFolder))
>-    throw "Failed: Bookmark for " + uri.spec + " not added to Bookmarks Menu";
>+  controller.assertJS("subject.placesAPI.isBookmarkInFolder(subject.URI, subject.bmksMenuFolder) == true",
>+                      {placesAPI: PlacesAPI, bmksMenuFolder: PlacesAPI.bookmarksService.bookmarksMenuFolder, URI: uri});

Dito.

>+++ b/firefox/testFindInPage/testFindInPage.js
>   // Check that the next result has been selected
>-  if (selectedText.getRangeAt(0).compareBoundaryPoints(comparator, range) == 0) {
>-    throw "Next search result has not been highlighted."
>-  }
>+  controller.assertJS("subject.searchResultStart.compareBoundaryPoints(subject.comparator, subject.range) != 0",
>+                      {searchResultStart: selectedText.getRangeAt(0), comparator: comparator, range: range});

Just use subject.isNextResult.

>-  if (selectedText.toString().toLowerCase() != searchTerm) {
>-    throw "Searched string does not match selected string."
>-  }
>+  controller.assertJS("subject.selection.toLowerCase() == subject.searchTerm",
>+                      {selection: selectedText.toString(), searchTerm: searchTerm});

What about subject.selectedText.

>-  if (selectedText.getRangeAt(0).compareBoundaryPoints(comparator, range) != 0) {
>-    throw "Previous search result has not been highlighted."
>-  }
>+  controller.assertJS("subject.searchResultStart.compareBoundaryPoints(subject.comparator, subject.range) == 0)",
>+                      {searchResultStart: selectedText.getRangeAt(0), comparator: comparator, range: range});

See above.

>+++ b/firefox/testPopups/testPopupsAllowed.js
>-  if (cssInfo.getPropertyValue('display') != "none") {
>-    throw "Status bar icon is visible";
>-  }
>+  controller.assertJS("subject.getPropertyValue('display') == 'none'", cssInfo);

subject.display?

>+  controller.assertJS("subject.windowCount != subject.windows.length",
>+                      {windowCount: windowCount, windows: mozmill.utils.getWindows()});

Can you use (pre/post)WindowCount here?

>+++ b/firefox/testPopups/testPopupsBlocked.js
>-  if (cssInfo.getPropertyValue('display') != "-moz-box") {
>-    throw "Status bar icon is not visible";
>-  }
>+  controller.assertJS("subject.getPropertyValue('display') == '-moz-box'", cssInfo);
> 
>   // Check that the window count has not changed
>-  if (windowCount != mozmill.utils.getWindows().length) {
>-    throw "Pop-ups were not blocked";
>-  }
>+  controller.assertJS("subject.windowCount != subject.windows.length",
>+                      {windowCount: windowCount, windows: mozmill.utils.getWindows()});

Dito.

>+++ b/firefox/testSecurity/testSafeBrowsingNotificationBar.js
>+	controller.assertJS("subject.urlbar.value.indexOf('http://www.stopbadware.org/') != -1",
>+	                    {urlbar: locationBar.getNode()});

Please move value out of the expression.

>+++ b/shared-modules/testPrefsAPI.js
>-    if(!callback)
>-      throw "No callback given for Preferences Dialog";
>+	controller.assertJS("subject != null", callback);

Here we need more context. You can leave it and simply throw an Error.

>+++ b/shared-modules/testPrivateBrowsingAPI.js
>-      if (!this._handler)
>-        throw new Error("Private Browsing mode not enabled due to missing callback handler");
>+	  controller.assertJS("subject != null", this._handler);

Please leave it and simply use "Missing callback handler."

>+++ b/shared-modules/testSearchAPI.js
>-    if(locationBar.getNode().value.indexOf(domainName) == -1)
>-      throw "Expected domain name doesn't match the current one"
>+	controller.assertJS("subject.locationBar.value.indexOf(subject.domain != -1",
>+	                    {locationBar: locationBar.getNode(), domain: domainName});

Use subject.matchDomainName. There is also a closing bracket missing.

>     // Check if search term is listed in URL
>-    if(locationBar.getNode().value.indexOf(searchTerm) == -1)
>-      throw "Search term in URL expected but not found.";
>+	controller.assertJS("subject.locationBar.value.indexOf(subject.searchTerm != -1",
>+	                    {locationBar: locationBar.getNode(), searchTerm: searchTerm});

Use subject.matchSearchTerm

>+++ b/shared-modules/testSoftwareUpdateAPI.js
>-      if (channel != prefs.getPref("app.update.channel", ""))
>-        throw new Error("Expected channel " + channel + "but got " + channelPref + "");
>+	  controller.assertJS("subject.channel == subject.preferences.getPref('app.update.channel','')",
>+	                      {channel: channel, preferences: prefs});

Please use currentChannel and expectedChannel.

>       // Only allow updates of the given type
>-      if (updateType != this.updateType)
>-        throw new Error("Expected update " + updateType + "but got " + this.updateType + "");
>+	  controller.assertJS("subject.expectedUpdateType == subject.updateType",
>+	                      {expectedUpdateType: updateType, updateType: this.updateType});

Similar here.

>+++ b/shared-modules/testToolbarAPI.js
>   getUnderlinedText : function autoCompleteResults_getUnderlinedText(result, type) {
>-    if (!result.getNode())
>-      throw new Error(arguments.callee.name + ": Result node is undefined.");
>+    controller.assertJS("subject != null", result.getNode());

Please more context.

>+++ b/shared-modules/testUtilsAPI.js
>   if (visibility) {
>-    if (state != 'visible')
>-      throw new Error("Element is hidden but should be visible");
>+    controller.assertJS("subject == 'visible'", state);
>   } else {
>-    if (state == 'visible')
>-      throw new Error("Element is visible but should be hidden");
>+    controller.assertJS("subject == 'hidden'", state);

Same here.

Comment 13

[Tobias (:Tobbi) Markus]

Attachment: v.1.1 Converting throw to assertJS statements, branch: 1.9.2

I think I've corrected everything you've caught now.

Comment 14

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

Comment on attachment 434197 [details] [diff] [review]
v.1.1 Converting throw to assertJS statements, branch: 1.9.2

As talked on IRC it will need another update.

Comment 15

[Tobias (:Tobbi) Markus]

Attachment: v.1.2 Converting throw to assertJS, branch 1.9.2

This is another reviewed patch.

Comment 16

[Tobias (:Tobbi) Markus]

Sorry for the indentation issue, all of my patch programmes seem to be messed up. 

Also, with reviewed I mean 'reviewed by me' of course.

Comment 17

[Tobias (:Tobbi) Markus]

Attachment: v.1.2 for 1.9.2 branch, converting throw to assertJS [checked-in]

Fixed the indentation now!

Comment 18

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

Comment on attachment 435129 [details] [diff] [review]
v.1.2 for 1.9.2 branch, converting throw to assertJS [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/a6997338b16e

Comment 19

[Tobias (:Tobbi) Markus]

Attachment: v.1.0 for mozilla1.9.1 branch, converting throw{} to assertJS

Comment 20

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

Landed last patch on 1.9.1:
http://hg.mozilla.org/qa/mozmill-tests/rev/7372f9365721

Thanks Tobbi for working on it!

Comment 21

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

Comment on attachment 435464 [details] [diff] [review]
v.1.0 for mozilla1.9.1 branch, converting throw{} to assertJS

Forgot about that.