586286 - Update Mozmill tests to remove deprecated assertJS calls

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

With bug 583773 fixed for Mozmill 1.4.2 we will have to update our tests so we do not have to use assertJS to test DOM properties (getAttribute call) anymore. This will make things much easier and compact. Here an example:

> - controller.assertJS("subject.getAttribute('oncommand') == 'restoreSession();'", button.getNode());
> + controller.assertDOMProperty(button, "oncommand", "restoreSession();"

I would like to wait until 1.4.2 has been released and proofed to be stable and secure until we start to work on this bug. Aaron, can you also cover that bug?

Comment 1

[Aaron Train [:aaronmt]]

We can cover this in the test refactoring unless there's a reason to do it now?

Comment 2

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

It's perfect for the test refactoring work.

Comment 3

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

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Comment 4

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

Anthony, that's the next and probably last bug about deprecated methods in Mozmill 2.0. Can you please schedule it accordingly to your current work load? Probably the shared modules should be fixed first. A bug doesn't exist yet. Thanks.

Comment 5

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

Bug 724713 to update the shared modules is done now. Can we get someone to update the tests accordingly? It would be good to get this done as soon as possible. Alex, Remus, or Vlad, would one of you have the time?

Comment 6

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

As talked with David on IRC he will take this bug so we can get it in ASAP and don't have to wait longer for an assignee.

Comment 7

[David Guo [:davidg]]

Attachment: Patch to replace assertJS v1

Comment 8

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

Comment on attachment 614606 [details] [diff] [review]
Patch to replace assertJS v1

># User David Guo <[e-mail]>
># Date 1334272935 25200
># Node ID 0353bd2351e013ffd97019ce3ced0ad60434c67e
># Parent  b287087cafbf4a1d16b433fe5bbd7b2b13f94293
>[mq]: bug-586286-fix

Please fix your email address and add a commit message to this patch.

>   // Confirm that 'mozilla' is in the locationbar and the awesomecomplete list is open
>-  controller.assertJS("subject.contains('" + TEST_STRING + "') == true", locationBar);
>+  expect.ok(locationBar.contains(TEST_STRING));

I would make use of expect.match() here instead of using the .contains method of the locationBar class.

Also make sure to add a message to the assertion methods as second argument which explains what we are testing. This applies to all the instances of expect/assert you make use of. This way we could get rid of some of the in-code comments.

>+++ b/tests/functional/testAwesomeBar/testFaviconInAutocomplete.js
>   // Check that both Favicons have the same URL
>-  controller.assertJS("subject.isSameFavicon == true",
>-                      {isSameFavicon: richlistItem.getNode().image.indexOf(locationBarFaviconUrl) != -1});
>+  expect.notEqual(richlistItem.getNode().image.indexOf(locationBarFaviconUrl), -1);

Not sure why we are doing index of here. I think we should compare both URLs by using expect.equal().

>-    controller.assertJS("subject.enteredTitle == subject.underlinedTitle",
>-                        {enteredTitle: LOCAL_TEST_PAGE.string, 
>-                         underlinedTitle: entry.toLowerCase()});
>+    expect.equal(LOCAL_TEST_PAGE.string, entry.toLowerCase());

I think we should directly make use of expect.match() here so we can also can get rid of the .toLowerCase() call and handle it through the regex.

>+++ b/tests/functional/testCookies/testEnableCookies.js
>+  expect.ok(cm.cookieExists({
>+    host: persisted.hostName,
>+    name: "litmus_1", 
>+    path: "/cookies/"
>+  }));

Please fix up the indentation of the code here. Make sure to put the first value of the hash into the first line.

>+++ b/tests/functional/testCookies/testRemoveCookie.js
>+  expect.ok(removed);

Please get rid of the temporary 'removed' variable and do it the same way as above.

>+++ b/tests/functional/testFindInPage/testFindInPage.js
>-  controller.assertJS("subject.selectedText == subject.searchTerm",
>-                      {selectedText: selectedText.toString().toLowerCase(), searchTerm: searchTerm});
>+  expect.equal(selectedText.toString().toLowerCase(), searchTerm);

Please use .match() here in all three cases.

>+++ b/tests/functional/testInstallation/testBreakpadInstalled.js
>   // Do we have the correct server URL?
>   var pos = crashReporter.serverURL.spec.search(states['ServerURLPattern']);
>-  controller.assertJS("subject.expectedCrashReportURL == true",
>-                      {expectedCrashReportURL: pos != -1});
>+  expect.notEqual(pos, -1);

I think we can do it more easily with expect.match().

>+++ b/tests/functional/testPasswordManager/testPasswordSavedAndDeleted.js
[..]
>   // Verify there is at least one saved password
>-  controller.assertJS("subject.view.rowCount == 1", signOnsTree);
>+  expect.equal(signOnsTree.view.rowCount, 1);

This should be an assert call instead. If no password is set the appropriate button wouldn't be enabled to delete all passwords and we end-up with a running modal dialog handler.

>+++ b/tests/functional/testPrivateBrowsing/testCloseWindow.js
>-  controller.assertJS("subject.tabs.length == subject.expectedCount",
>-                      {tabs: controller.tabs, expectedCount: (LOCAL_TEST_PAGES.length + 1)});
>+  expect.ok(controller.tabs.length, LOCAL_TEST_PAGES.length + 1);

You want to call expect.equal() here. Please also put brackets around computations.

>+++ b/tests/functional/testPrivateBrowsing/testStartStopPBMode.js
>   // Title modifier should have been set
>-  controller.assertJS("subject.hasTitleModifier == true",
>-                      {hasTitleModifier: controller.window.document.
>-                                         title.indexOf(modifier) != -1});
>+  expect.notEqual(controller.window.document.title.indexOf(modifier), -1);

You wanna use expect.match() here.

>   // All tabs should be restored
>-  controller.assertJS("subject.allTabsRestored == true",
>-                      {allTabsRestored: controller.tabs.length == LOCAL_TEST_PAGES.length + 1});
>+  expect.equal(controller.tabs.length, LOCAL_TEST_PAGES.length + 1);

As above please brackets.

>   // No title modifier should have been set
>-  controller.assertJS("subject.noTitleModifier == true",
>-                      {noTitleModifier: controller.window.document.
>-                                        title.indexOf(modifier) == -1});
>+  expect.equal(controller.window.document.title.indexOf(modifier), -1);

Same as above. Please use expect.match().

>+++ b/tests/functional/testPrivateBrowsing/testTabRestoration.js
>-  controller.assertJS("subject.tabCountActual == subject.tabCountExpected",
>-                      {tabCountActual: controller.tabs.length,
>-                       tabCountExpected: LOCAL_TEST_PAGES.length + 1});
>+  expect.equal(controller.tabs.length, LOCAL_TEST_PAGES.length + 1);

Again.

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

expect.match() please.

>+++ b/tests/functional/testSecurity/testUnknownIssuer.js
>-  controller.assertJS("subject.errorMessage.indexOf('sec_error_unknown_issuer') != -1",
>-                      {errorMessage: text.getNode().textContent});
>+  expect.notEqual(text.getNode().textContent.indexOf('sec_error_unknown_issuer'), -1);
> }

expect.match() please.

Comment 9

[David Guo [:davidg]]

> Not sure why we are doing index of here. I think we should compare both URLs by
> using expect.equal().

Couldn't get it to work with .equal or .match. 

The two strings are moz-anno:favicon:http://localhost:43336/images/mozilla_favicon.ico and http://localhost:43336/images/mozilla_favicon.ico

Comment 10

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

Ok, I can see the problem now. Even if we would use the second value as reference we can't easily make it a regex because the '.' would have to be escaped. For cases like those we probably really need a contains() method in the assertion module.

Comment 11

[David Guo [:davidg]]

Attachment: Patch to replace assertJS v2

Comment 12

[David Guo [:davidg]]

Thanks for the review. The only other thing I did not end up doing was the following line:

> expect.equal(controller.window.document.title.indexOf(modifier), -1);

Where modifier is (Private Browsing). I used notMatch but that did not seem to work.

Comment 13

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

Which regex have you used? Keep in mind that you have to escape the brackets.

Comment 14

[David Guo [:davidg]]

The regex is from
> modifier = controller.window.document.documentElement.getAttribute("titlemodifier_privatebrowsing");

So I'm not sure if it's easy to escape.

Comment 15

[David Guo [:davidg]]

Attachment: Patch to replace assertJS v3

Comment 16

[David Guo [:davidg]]

Attachment: Patch to replace assertJS v3b

Fixed e-mail

Comment 17

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

Comment on attachment 615884 [details] [diff] [review]
Patch to replace assertJS v3b

>+Expect.prototype.contain = function Expect_contain(aString, aPattern, aMessage) {
>+Expect.prototype.notContain = function Expect_notContain(aString, aPattern, aMessage) {

Please file a separate bug so we can get this added to Mozmill 2.0. Same for the API refactor which also needs this update. You can do that via a pull request.

>+++ b/tests/functional/testAwesomeBar/testEscapeAutocomplete.js
>-  controller.assertJS("subject.contains('" + TEST_STRING + "') == true", locationBar);
>+  expect.contain(locationBar.value, TEST_STRING,
>+                 'The word mozilla is in the locationbar');

I wouldn't mind if you shorten some of the messages. Here it would be fine to use "Search string found in location bar". There is no need to reference the test string because it would require several updates to different places. Also please use double quotes around the message.

>-  controller.assertJS("subject.autoCompleteResults.isOpened == true", locationBar);
>+  expect.ok(locationBar.autoCompleteResults.isOpened,
>+            'The awesomecomplete list is open');

Please call it auto-complete list. 'awesome' was just a marketing thing and we shouldn't use it here.

>-  controller.assertJS("subject.contains('" + TEST_STRING + "') == true", locationBar);
>+  expect.contain(locationBar.value, TEST_STRING,
>+                 'The word mozilla is in the locationbar');
>-  controller.assertJS("subject.autoCompleteResults.isOpened == false", locationBar);
>+  expect.ok(!locationBar.autoCompleteResults.isOpened,
>+            'The awesomecomplete list is closed');

Same as above.

>+++ b/tests/functional/testAwesomeBar/testFaviconInAutocomplete.js
>+  expect.contain(richlistItem.getNode().image, locationBarFaviconUrl,
>+                 'Check that both Favicons have the same URL');

This message doesn't tell me anything, but "Favicons in location bar and auto-complete list are identical" does.

>+++ b/tests/functional/testAwesomeBar/testSuggestHistoryBookmarks.js
>   var autoCompleteResultsList = locationBar.autoCompleteResults.getElement({type:"results"});
>-  controller.assertJS("subject.getNumberOfVisibleRows() == 1", autoCompleteResultsList.getNode());
>+  expect.equal(autoCompleteResultsList.getNode().getNumberOfVisibleRows(), 1,
>+               'There is one visible result on the autocomplete list');

Is the result from the above method different from the autoCompleteResults.visibleResults() method?

>+  expect.contain(richlistItem.getNode().getAttribute('type'), 'bookmark',
>+                 'The bookmark star is present');

Actually that doesn't check if the star is present but if the item is a bookmark.

>+++ b/tests/functional/testBookmarks/testAddBookmarkToMenu.js
>-  // Fail if the URI is already bookmarked
>-  controller.assertJS("subject.isBookmarked == false", {
>-    isBookmarked: places.bookmarksService.isBookmarked(uri)
>-  });
>+  expect.ok(!places.bookmarksService.isBookmarked(uri),
>+            'URI is not already bookmarked');

This has to be an assert but it shouldn't be necessary if we really reset the bookmarks in setupModule. This code has been added before the places method was available. So we probably can get rid of it now.

>+  expect.ok(places.isBookmarkInFolder(uri, places.bookmarksService.bookmarksMenuFolder),
>+            'Bookmark was created in the bookmarks menu');

Can you please move the bookmark check into it's own line and assign it to a new variable? That would make it way better readable.

>+++ b/tests/functional/testCookies/testEnableCookies.js
>+  expect.ok(cm.cookieExists({host: persisted.hostName,
>+    name: "litmus_1", 
>+    path: "/cookies/"
>+  }, 'The single cookie is saved.'));

Same here. This is barely readable and has still a wrong indentation.

>+++ b/tests/functional/testCookies/testRemoveAllCookies.js
>-  controller.assertJS("subject.cookieCount > 0",
>-                      {cookieCount: cookiesList.view.rowCount});
>+  expect.ok(cookiesList.view.rowCount > 0, 'There are cookies in the list.');

This should be an assert because the remove all button will be disabled otherwise.

>+++ b/tests/functional/testCookies/testRemoveCookie.js
>+  expect.ok(!cm.cookieExists({host: persisted.hostName,
>     name: "litmus_1", 
>     path: "/cookies/"
>-  });
>+  }, 'The cookie has been removed'));

See above.

>-  controller.assertJS("subject.isCookieRemoved == true", {
>-    isCookieRemoved: removed
>-  });
>-  
>-  controller.assertJS("subject.list.view.rowCount == subject.numberCookies", {
>-    list: cookiesList, 
>-    numberCookies: origNumCookies - 1
>-  });
>+  expect.equal(cookiesList.view.rowCount, origNumCookies - 1,
>+               'There is one less cookie than before');

We do no longer check if the front-end is in sync with the back-end. Also put brackets around computations.

>+++ b/tests/functional/testFindInPage/testFindInPage.js
>-  // Check that the next result has been selected
>-  controller.assertJS("subject.isNextResult == true",
>-                      {isNextResult: selectedText.getRangeAt(0).compareBoundaryPoints(comparator, range) != 0});
>+  expect.notEqual(selectedText.getRangeAt(0).compareBoundaryPoints(comparator, range), 0,
>+                  'The next result has not been selected');

Please fix the message which is reverted now. Also it would help to introduce a new variable here.

>+  expect.equal(selectedText.getRangeAt(0).compareBoundaryPoints(comparator, range), 0,
>+               'The first result has been selected again');

Same as above for the new variable.

>+++ b/tests/functional/testInstallation/testBreakpadInstalled.js
>+  expect.match(crashReporter.serverURL.spec, states['ServerURLPattern'],
>+               'The server URL is correct');

Which server URL? Please add 'breakpad' at least.

>+++ b/tests/functional/testPasswordManager/testPasswordSavedAndDeleted.js
>+var { assert, expect } = require("../../../lib/assertions");

Why both if you don't use assert?

>-  // Verify there is at least one saved password
>-  controller.assertJS("subject.view.rowCount == 1", signOnsTree);
>+  assert.equal(signOnsTree.view.rowCount, 1,
>+               'There is a saved password');

There is enough space to have it in a single line. And same here as for cookies. Should probably be an assert because the button would be disabled otherwise.

>+++ b/tests/functional/testPrivateBrowsing/testGeolocation.js
>-    controller.assertJS("subject.hasGeoTokens == true",
>-                        {hasGeoTokens: tokens.value > 0});
>+    expect.ok(tokens.value > 0);

Please add a message.

>-  controller.assertJS("subject.hasNoGeoTokens == true",
>-                      {hasNoGeoTokens: tokens.value == 0});
>+  expect.equal(tokens.value, 0);

Here too.

>+++ b/tests/functional/testPrivateBrowsing/testStartStopPBMode.js
>+  expect.contain(controller.window.document.title, modifier,
>+                 'Title modifier should be set');

"Title modifier has been set" please.

>+  expect.equal(controller.tabs.length, (LOCAL_TEST_PAGES.length + 1),
>+               'All tabs should be restored');

Same here. Please remove 'should'.

>+  expect.notContain(controller.window.document.title, modifier,
>+                    'Title modifier should not have been set');

And again.

>+++ b/tests/functional/testPrivateBrowsing/testTabRestoration.js
>+  expect.equal(controller.tabs.length, (LOCAL_TEST_PAGES.length + 1),
>+               'All tabs should be restored');

And again.

>+++ b/tests/functional/testPrivateBrowsing/testTabsDismissedOnStop.js
>-  // All tabs should have been removed
>-  controller.assertJS("subject.tabs.length == 1", controller);
>+  expect.equal(controller.tabs.length, 1,
>+               'All tabs should have been removed');

And again.

>+++ b/tests/functional/testSearch/testGetMoreSearchEngines.js
>-  // Check that the search engine is not installed yet
>-  controller.assertJS("subject.isEngineInstalled == false",
>-                      {isEngineInstalled: searchBar.isEngineInstalled(searchEngine.name)});
>+  expect.ok(!searchBar.isEngineInstalled(searchEngine.name),
>+            'The specified search engine has not been installed');

This has to be an assert. Otherwise we fail with an unhandled exception.

>+++ b/tests/functional/testSearch/testRemoveSearchEngine.js
>-  controller.assertJS("subject.enginesCount > 1",
>-                      {enginesCount: engines.length});
>+  expect.ok(engines.length > 1, 'There are more than one search engines');

Please fix the message.

>+++ b/tests/functional/testSearch/testSearchSelection.js
>-  controller.assertJS("subject.isEngineNameInContextMenu == true",
>-                      {isEngineNameInContextMenu: contextLabel.indexOf(engineName) != -1});
>+  expect.contain(contextLabel, engineName, 'The specified search engine exists');

Can you update the message to 'is installed'?

Comment 18

[David Guo [:davidg]]

> Can you please move the bookmark check into it's own line and assign it to a new
> variable? That would make it way better readable.

Can you be more specific? I've tried it out by moving the boolean into a new line as a variable, but it does not make it anymore better.

> We do no longer check if the front-end is in sync with the back-end.

Can you clarify what you mean by this?

> Why both if you don't use assert?

>>-  // Verify there is at least one saved password
>>-  controller.assertJS("subject.view.rowCount == 1", signOnsTree);
>>+  assert.equal(signOnsTree.view.rowCount, 1,
>>+               'There is a saved password');

> There is enough space to have it in a single line. And same here as for cookies.
> Should probably be an assert because the button would be disabled otherwise.

Did you misread the code or did I not use assert correctly?

Comment 19

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

(In reply to David Guo [:davidg] from comment #18)
> > Can you please move the bookmark check into it's own line and assign it to a new
> > variable? That would make it way better readable.
> 
> Can you be more specific? I've tried it out by moving the boolean into a new
> line as a variable, but it does not make it anymore better.

Would be nice if you could leave in more context. It's not clear to which code this refers to.

So I mean:

>+  let exists = places.isBookmarkInFolder(uri, places.bookmarksService.bookmarksMenuFolder)
>+  expect.ok(exists, 'Bookmark was created in the bookmarks menu');

> > We do no longer check if the front-end is in sync with the back-end.
> 
> Can you clarify what you mean by this?

Same here as above. So you have removed one of the asserts as you can see when checking the diff.

> > Why both if you don't use assert?
> 
> >>-  // Verify there is at least one saved password
> >>-  controller.assertJS("subject.view.rowCount == 1", signOnsTree);
> >>+  assert.equal(signOnsTree.view.rowCount, 1,
> >>+               'There is a saved password');
> 
> > There is enough space to have it in a single line. And same here as for cookies.
> > Should probably be an assert because the button would be disabled otherwise.
> 
> Did you misread the code or did I not use assert correctly?

Oh, yes. Failure on my side. Sorry.

Comment 20

[David Guo [:davidg]]

Attachment: Patch to replace assertJS v4

Comment 21

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

Comment on attachment 617668 [details] [diff] [review]
Patch to replace assertJS v4

Please read my review comments carefully and ensure to cover each of those. If necessary and I would propose to add replies to each of those (if they have been addressed), so you can't miss them.

>-                      {isItemBookmarked: richlistItem.getNode().getAttribute('type').indexOf('bookmark') != -1});
>+  expect.contain(richlistItem.getNode().getAttribute('type'), 'bookmark',
>+                 'The auto-complete result is a bookmark');

In JS we use double quotes by default. And we should stick by that. So please update those instances.

I haven't checked yet, if other review comments are missing too.

Comment 22

[David Guo [:davidg]]

Attachment: Patch to replace assertJS v5

Comment 23

[David Guo [:davidg]]

Attachment: Manifest for tests that have changed

Comment 24

[David Guo [:davidg]]

Attachment: Patch to replace assertJS v6

Comment 25

[David Guo [:davidg]]

Sorry about that. Double checked to make sure all your other concerns were met and also added a manifest to be able to verify that this update does not change test statuses.

Comment 26

[David Guo [:davidg]]

Attachment: Patch to replace assertJS v7

Removed unnecessary style changes to make reviewing easier.

Comment 27

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

Comment on attachment 618200 [details] [diff] [review]
Patch to replace assertJS v7

+++ b/tests/functional/testAwesomeBar/testSuggestHistoryBookmarks.js

-  var autoCompleteResultsList = locationBar.autoCompleteResults.getElement({type:"results"});
-  controller.assertJS("subject.getNumberOfVisibleRows() == 1", autoCompleteResultsList.getNode());
+  expect.equal(locationBar.autoCompleteResults.visibleResults.length, 1,
+               "There is one visible result on the autocomplete list");

nit: 'in' the ...
 
+++ b/tests/functional/testBookmarks/testAddBookmarkToMenu.js

+  expect.ok(places.isBookmarkInFolder(uri, places.bookmarksService.bookmarksMenuFolder),
+            "Bookmark was created in the bookmarks menu");

This one hasn't been addressed yet. Please move this complex function call into it's own line and assign it to a variable.

+++ b/tests/functional/testCookies/testEnableCookies.js

+  expect.ok(cm.cookieExists({host: persisted.hostName,
+                             name: "litmus_1", 
+                             path: "/cookies/" }, 
+            "The single cookie is saved."));

Same here. Please move the cm.cookieExists() to its own line.

+++ b/tests/functional/testCookies/testRemoveCookie.js

-  var removed = !cm.cookieExists({
-    host: persisted.hostName, 
-    name: "litmus_1", 
-    path: "/cookies/"
-  });
+  expect.ok(!cm.cookieExists({host: persisted.hostName,
+                              name: "litmus_1", 
+                              path: "/cookies/" }, 
+            "The cookie has been removed"));

And here. It was just fine before.
 
+++ b/tests/functional/testFindInPage/testFindInPage.js

+  var isNextResult = selectedText.getRangeAt(0).compareBoundaryPoints(comparator, range);
+  expect.notEqual(isNextResult, 0, "The next result has been selected");

Is the 'is' prefix for the variable correct? What does compareBoundaryPoints() return?

+++ b/tests/functional/testPrivateBrowsing/testGeolocation.js

+    expect.ok(tokens.value > 0, "Geo access tokens present");

What is "Geo"? Please you use the whole name.

+  expect.equal(tokens.value, 0, "No geo access token present");

Same here.

+  expect.contain(controller.window.document.title, modifier,
+                 "Title modifier has be set");

"has been set"

+  expect.notContain(controller.window.document.title, modifier,
+                    "Title modifier has not have been set");

Strange wording...

+++ b/tests/functional/testPrivateBrowsing/testTabsDismissedOnStop.js

+  expect.equal(controller.tabs.length, 1,
+               "All tabs have been removed");

Why no single line?

+++ b/tests/functional/testSecurity/testSecurityNotification.js

+  expect.contain(text.getNode().textContent, "ssl_error_bad_cert_domain",
+                 "The error code should be a SSL Bad Cert Error");

There is also an unnecessary 'should'. We strictly check here. So please remove it and please check for other instances.

+++ b/tests/functional/testSecurity/testUnknownIssuer.js

+  expect.contain(text.getNode().textContent, "sec_error_unknown_issuer",
+                 "The error code should be a unknown issuer error");

Same here

Comment 28

[David Guo [:davidg]]

(In reply to Henrik Skupin (:whimboo) from comment #27)
> Comment on attachment 618200 [details] [diff] [review]
> Patch to replace assertJS v7
> 
> +++ b/tests/functional/testAwesomeBar/testSuggestHistoryBookmarks.js
> 
> -  var autoCompleteResultsList =
> locationBar.autoCompleteResults.getElement({type:"results"});
> -  controller.assertJS("subject.getNumberOfVisibleRows() == 1",
> autoCompleteResultsList.getNode());
> +  expect.equal(locationBar.autoCompleteResults.visibleResults.length, 1,
> +               "There is one visible result on the autocomplete list");
> 
> nit: 'in' the ...
>

Done.

> +++ b/tests/functional/testBookmarks/testAddBookmarkToMenu.js
> 
> +  expect.ok(places.isBookmarkInFolder(uri,
> places.bookmarksService.bookmarksMenuFolder),
> +            "Bookmark was created in the bookmarks menu");
> 
> This one hasn't been addressed yet. Please move this complex function call
> into it's own line and assign it to a variable.
>

Done.

> +++ b/tests/functional/testCookies/testEnableCookies.js
> 
> +  expect.ok(cm.cookieExists({host: persisted.hostName,
> +                             name: "litmus_1", 
> +                             path: "/cookies/" }, 
> +            "The single cookie is saved."));
> 
> Same here. Please move the cm.cookieExists() to its own line.
> 
> +++ b/tests/functional/testCookies/testRemoveCookie.js
> 
> -  var removed = !cm.cookieExists({
> -    host: persisted.hostName, 
> -    name: "litmus_1", 
> -    path: "/cookies/"
> -  });
> +  expect.ok(!cm.cookieExists({host: persisted.hostName,
> +                              name: "litmus_1", 
> +                              path: "/cookies/" }, 
> +            "The cookie has been removed"));
> 
> And here. It was just fine before.

You were the one that suggested to change it to this style in an earlier review :P. Done.

>
> +++ b/tests/functional/testFindInPage/testFindInPage.js
> 
> +  var isNextResult =
> selectedText.getRangeAt(0).compareBoundaryPoints(comparator, range);
> +  expect.notEqual(isNextResult, 0, "The next result has been selected");
> 
> Is the 'is' prefix for the variable correct? What does
> compareBoundaryPoints() return?
>

I suppose I could change it to resultPosition.

> +++ b/tests/functional/testPrivateBrowsing/testGeolocation.js
> 
> +    expect.ok(tokens.value > 0, "Geo access tokens present");
> 
> What is "Geo"? Please you use the whole name.
> 
> +  expect.equal(tokens.value, 0, "No geo access token present");
> 
> Same here.
>

Done. Also, these tests are currently commented out but should they be there at all? Or maybe they should be changed to be skipped instead.

> +  expect.contain(controller.window.document.title, modifier,
> +                 "Title modifier has be set");
> 
> "has been set"
>
> +  expect.notContain(controller.window.document.title, modifier,
> +                    "Title modifier has not have been set");
> 
> Strange wording...
>

Thanks for catching these.
 
> +++ b/tests/functional/testPrivateBrowsing/testTabsDismissedOnStop.js
> 
> +  expect.equal(controller.tabs.length, 1,
> +               "All tabs have been removed");
> 
> Why no single line?
> 
> +++ b/tests/functional/testSecurity/testSecurityNotification.js
> 
> +  expect.contain(text.getNode().textContent, "ssl_error_bad_cert_domain",
> +                 "The error code should be a SSL Bad Cert Error");
> 
> There is also an unnecessary 'should'. We strictly check here. So please
> remove it and please check for other instances.
> 
> +++ b/tests/functional/testSecurity/testUnknownIssuer.js
> 
> +  expect.contain(text.getNode().textContent, "sec_error_unknown_issuer",
> +                 "The error code should be a unknown issuer error");
> 
> Same here

Thanks for catching these as well. They were changed as you can see in patch v6, but the regressed since the rollback.

I will double check everything before posting the new patch. Thanks for the review!

Comment 29

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

(In reply to David Guo [:davidg] from comment #28)
> > +  expect.ok(!cm.cookieExists({host: persisted.hostName,
> > +                              name: "litmus_1", 
> > +                              path: "/cookies/" }, 
> > +            "The cookie has been removed"));
> > 
> > And here. It was just fine before.
> 
> You were the one that suggested to change it to this style in an earlier
> review :P. Done.

And I reverted it in my 2nd last review given that was too hard to read.

> > +  var isNextResult =
> > selectedText.getRangeAt(0).compareBoundaryPoints(comparator, range);
> > +  expect.notEqual(isNextResult, 0, "The next result has been selected");
> > 
> > Is the 'is' prefix for the variable correct? What does
> > compareBoundaryPoints() return?
> >
> 
> I suppose I could change it to resultPosition.

Probably this can help which gets called by compareBoundaryPoints:
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentUtils.h#351

> > +++ b/tests/functional/testPrivateBrowsing/testGeolocation.js
> > 
> > +    expect.ok(tokens.value > 0, "Geo access tokens present");
> > 
> > What is "Geo"? Please you use the whole name.
> > 
> > +  expect.equal(tokens.value, 0, "No geo access token present");
> > 
> > Same here.
> >
> 
> Done. Also, these tests are currently commented out but should they be there
> at all? Or maybe they should be changed to be skipped instead.

Isn't there an XXX comment? This should be because of bug 683157.

Comment 30

[David Guo [:davidg]]

> Isn't there an XXX comment? This should be because of bug 683157.

Correct. The comments say that it is because of bug 685085, but that's blocked by 683157.

Comment 31

[David Guo [:davidg]]

(In reply to David Guo [:davidg] from comment #30)
> > Isn't there an XXX comment? This should be because of bug 683157.
> 
> Correct. The comments say that it is because of bug 685085, but that's
> blocked by 683157.

Typo, it's supposed to be bug 685805.

Comment 32

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

Just leave it as it is.

Comment 33

[David Guo [:davidg]]

Attachment: Patch to replace assertJS v8

Comment 34

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

David is out this week, so Dave want to help out with the remaining items. I will now do a review on it.

Comment 35

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

Comment on attachment 618893 [details] [diff] [review]
Patch to replace assertJS v8

>+++ b/tests/functional/testPrivateBrowsing/testDownloadManagerClosed.js

>-    dm.controller.assertJS("subject.isCorrectDownload == true",
>-                           {isCorrectDownload: download.getNode().getAttribute('uri') == DOWNLOADS[i]});
>+    expect.equal(download.getNode().getAttribute("uri"), DOWNLOADS[i],
>+                 "Downloaded files have the same name");

We do not directly compare the name but check that the wanted file has been downloaded. So that should be reflected in the message.

Dave, can you please also check if the patch still applies cleanly on default and other branches down to esr10?

r=me with the above nit fixed.

Comment 36

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

Attachment: Patch to replace assertJS v9 (default) [checked-in]

Addressed nit, and updated to apply cleanly with default and mozilla-aurora. I will attach versions for the remaining branches.

Comment 37

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

Attachment: Patch to replace assertJS v9 (mozilla-beta)

Comment 38

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

Attachment: Patch to replace assertJS v9 (mozilla-release)

Comment 39

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

Attachment: Patch to replace assertJS v9 (mozilla-esr10)

Comment 40

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

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/4e49d7053849

Once everything works as expected I will check the other patches and get those landed.

Comment 41

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

Attachment: Followup to fix invalid syntax. v1.0 [checked-in]

I will update the branch patches appropriately.

Comment 42

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

Attachment: Patch to replace assertJS v9.1 (mozilla-beta) [checked-in]

Comment 43

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

Attachment: Patch to replace assertJS v9.1 (mozilla-release) [checked-in]

Comment 44

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

Attachment: Patch to replace assertJS v9.1 (mozilla-esr10) [checked-in]

Comment 45

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

Comment on attachment 620744 [details] [diff] [review]
Followup to fix invalid syntax. v1.0 [checked-in]

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/92215f47f1ae

Comment 46

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

Comment on attachment 620747 [details] [diff] [review]
Patch to replace assertJS v9.1 (mozilla-beta) [checked-in]

Marking remaining patches as obsolete for now because they also require an update.

Comment 47

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

(In reply to Henrik Skupin (:whimboo) from comment #46)
> Comment on attachment 620747 [details] [diff] [review]
> Patch to replace assertJS v9.1 (mozilla-beta)
> 
> Marking remaining patches as obsolete for now because they also require an
> update.

No, I have already updated them all...

Comment 48

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

Dave, I miss an updated (combined patch) for aurora. Can you please come up with one ASAP? Thanks.

Comment 49

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

Attachment: Patch to replace assertJS v9.1 (mozilla-aurora) [checked-in]

Comment 50

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

Comment on attachment 621032 [details] [diff] [review]
Patch to replace assertJS v9.1 (mozilla-aurora) [checked-in]

Looks good except the commit message which I have updated.

Comment 51

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

Landed patches on other branches:
http://hg.mozilla.org/qa/mozmill-tests/rev/f3156f57cc6b (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/1e8d5fe44991 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/58d7f2be78df (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/5228428275f3 (esr10)