Update Mozmill tests to remove deprecated assertJS calls

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: whimboo, Assigned: davehunt)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox12 fixed, firefox13 fixed, firefox14 fixed, firefox15 fixed, firefox-esr10 fixed, status1.9.2 wontfix)

Details

(Whiteboard: [lib][mozmill-refactor][qa-])

Attachments

(7 attachments, 12 obsolete attachments)

1.90 KB, patch
Details | Diff | Splinter Review
63.23 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
2.27 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
63.18 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
63.09 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
66.63 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
63.22 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

7 years ago
Blocks: 585086
(Reporter)

Updated

7 years ago
Whiteboard: [good first bug]
(Reporter)

Updated

7 years ago
No longer blocks: 585086
(Reporter)

Updated

7 years ago
Blocks: 596023
We can cover this in the test refactoring unless there's a reason to do it now?
(Reporter)

Comment 2

7 years ago
It's perfect for the test refactoring work.
(Reporter)

Comment 3

7 years ago
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.
Component: Mozmill Tests → Mozmill Tests
Product: Testing → Mozilla QA
Version: Trunk → unspecified
Whiteboard: [good first bug] → [good-first-bug][mozmill-test-refactor]
(Reporter)

Updated

6 years ago
Whiteboard: [good-first-bug][mozmill-test-refactor] → [good first bug][mozmill-test-refactor]
(Reporter)

Comment 4

6 years ago
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.
Summary: Update Mozmill tests to use assertDOMProperty instead of assertJS → Update Mozmill tests to remove deprecated assertJS calls
Depends on: 724713
Whiteboard: [good first bug][mozmill-test-refactor] → [good first bug][mozmill-refactor]
(Reporter)

Comment 5

6 years ago
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?
Blocks: 742550
(Reporter)

Updated

6 years ago
No longer blocks: 742550
(Reporter)

Updated

5 years ago
Blocks: 744007
(Reporter)

Comment 6

5 years ago
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.
Assignee: nobody → david.guo
Status: NEW → ASSIGNED

Comment 7

5 years ago
Created attachment 614606 [details] [diff] [review]
Patch to replace assertJS v1
Attachment #614606 - Flags: review?(hskupin)
(Reporter)

Comment 8

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

># User David Guo <dguo@moilla.com>
># 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.
Attachment #614606 - Flags: review?(hskupin) → review-

Comment 9

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

Comment 10

5 years ago
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

5 years ago
Created attachment 615573 [details] [diff] [review]
Patch to replace assertJS v2
Attachment #614606 - Attachment is obsolete: true

Comment 12

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

Comment 13

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

Comment 14

5 years ago
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

5 years ago
Created attachment 615859 [details] [diff] [review]
Patch to replace assertJS v3
Attachment #615859 - Flags: review?(hskupin)

Updated

5 years ago
Attachment #615573 - Attachment is obsolete: true

Comment 16

5 years ago
Created attachment 615884 [details] [diff] [review]
Patch to replace assertJS v3b

Fixed e-mail
Attachment #615859 - Attachment is obsolete: true
Attachment #615859 - Flags: review?(hskupin)

Updated

5 years ago
Attachment #615884 - Flags: review?(hskupin)
(Reporter)

Updated

5 years ago
Whiteboard: [good first bug][mozmill-refactor] → [lib]
(Reporter)

Updated

5 years ago
Whiteboard: [lib] → [lib][mozmill-refactor]
(Reporter)

Comment 17

5 years ago
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'?
Attachment #615884 - Flags: review?(hskupin) → review-

Comment 18

5 years ago
> 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?
(Reporter)

Comment 19

5 years ago
(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

5 years ago
Created attachment 617668 [details] [diff] [review]
Patch to replace assertJS v4
Attachment #615884 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #617668 - Flags: review?(hskupin)
(Reporter)

Comment 21

5 years ago
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.
Attachment #617668 - Flags: review?(hskupin) → review-

Comment 22

5 years ago
Created attachment 617966 [details] [diff] [review]
Patch to replace assertJS v5
Attachment #617668 - Attachment is obsolete: true

Updated

5 years ago
Attachment #617966 - Attachment is obsolete: true

Comment 23

5 years ago
Created attachment 617977 [details] [diff] [review]
Manifest for tests that have changed

Comment 24

5 years ago
Created attachment 617997 [details] [diff] [review]
Patch to replace assertJS v6
Attachment #617997 - Flags: review?(hskupin)

Comment 25

5 years ago
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

5 years ago
Created attachment 618200 [details] [diff] [review]
Patch to replace assertJS v7

Removed unnecessary style changes to make reviewing easier.
Attachment #617997 - Attachment is obsolete: true
Attachment #618200 - Flags: review?(hskupin)
Attachment #617997 - Flags: review?(hskupin)
(Reporter)

Comment 27

5 years ago
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
Attachment #618200 - Flags: review?(hskupin) → review-

Comment 28

5 years ago
(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!
(Reporter)

Comment 29

5 years ago
(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

5 years ago
> 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

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

Comment 32

5 years ago
Just leave it as it is.

Comment 33

5 years ago
Created attachment 618893 [details] [diff] [review]
Patch to replace assertJS v8
Attachment #618200 - Attachment is obsolete: true
Attachment #618893 - Flags: review?(hskupin)
(Reporter)

Comment 34

5 years ago
David is out this week, so Dave want to help out with the remaining items. I will now do a review on it.
Assignee: david.guo → dave.hunt
(Reporter)

Comment 35

5 years ago
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.
Attachment #618893 - Flags: review?(hskupin) → review+
(Assignee)

Comment 36

5 years ago
Created attachment 620656 [details] [diff] [review]
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.
Attachment #618893 - Attachment is obsolete: true
Attachment #620656 - Flags: review?(hskupin)
(Assignee)

Comment 37

5 years ago
Created attachment 620658 [details] [diff] [review]
Patch to replace assertJS v9 (mozilla-beta)
Attachment #620658 - Flags: review?(hskupin)
(Assignee)

Comment 38

5 years ago
Created attachment 620660 [details] [diff] [review]
Patch to replace assertJS v9 (mozilla-release)
Attachment #620660 - Flags: review?(hskupin)
(Assignee)

Comment 39

5 years ago
Created attachment 620662 [details] [diff] [review]
Patch to replace assertJS v9 (mozilla-esr10)
Attachment #620662 - Flags: review?(hskupin)
(Reporter)

Updated

5 years ago
Attachment #620656 - Flags: review?(hskupin) → review+
(Reporter)

Comment 40

5 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox-esr10: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 41

5 years ago
Created attachment 620744 [details] [diff] [review]
Followup to fix invalid syntax. v1.0 [checked-in]

I will update the branch patches appropriately.
Attachment #620744 - Flags: review?(hskupin)
(Assignee)

Comment 42

5 years ago
Created attachment 620747 [details] [diff] [review]
Patch to replace assertJS v9.1 (mozilla-beta) [checked-in]
Attachment #620658 - Attachment is obsolete: true
Attachment #620747 - Flags: review?(hskupin)
Attachment #620658 - Flags: review?(hskupin)
(Assignee)

Comment 43

5 years ago
Created attachment 620756 [details] [diff] [review]
Patch to replace assertJS v9.1 (mozilla-release) [checked-in]
Attachment #620660 - Attachment is obsolete: true
Attachment #620756 - Flags: review?(hskupin)
Attachment #620660 - Flags: review?(hskupin)
(Assignee)

Comment 44

5 years ago
Created attachment 620757 [details] [diff] [review]
Patch to replace assertJS v9.1 (mozilla-esr10) [checked-in]
Attachment #620662 - Attachment is obsolete: true
Attachment #620662 - Flags: review?(hskupin)
(Reporter)

Comment 45

5 years ago
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
Attachment #620744 - Attachment description: Followup to fix invalid syntax. v1.0 → Followup to fix invalid syntax. v1.0 [checked-in]
Attachment #620744 - Flags: review?(hskupin) → review+
(Reporter)

Comment 46

5 years ago
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.
Attachment #620747 - Attachment is obsolete: true
Attachment #620747 - Flags: review?(hskupin)
(Reporter)

Updated

5 years ago
Attachment #620756 - Attachment is obsolete: true
Attachment #620756 - Flags: review?(hskupin)
(Reporter)

Updated

5 years ago
Attachment #620757 - Attachment is obsolete: true
(Assignee)

Comment 47

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

Updated

5 years ago
Attachment #620756 - Attachment is obsolete: false
(Assignee)

Updated

5 years ago
Attachment #620756 - Flags: review?(hskupin)
(Assignee)

Updated

5 years ago
Attachment #620747 - Attachment is obsolete: false
Attachment #620747 - Flags: review?(hskupin)
(Assignee)

Updated

5 years ago
Attachment #620757 - Attachment description: Patch to replace assertJS v9 (mozilla-esr10) → Patch to replace assertJS v9.1 (mozilla-esr10)
Attachment #620757 - Attachment is obsolete: false
Attachment #620757 - Flags: review?(hskupin)
(Reporter)

Comment 48

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

Updated

5 years ago
Attachment #620656 - Attachment description: Patch to replace assertJS v9 (default, mozilla-aurora) → Patch to replace assertJS v9 (default) [checked-in]
(Assignee)

Comment 49

5 years ago
Created attachment 621032 [details] [diff] [review]
Patch to replace assertJS v9.1 (mozilla-aurora) [checked-in]
Attachment #621032 - Flags: review?(hskupin)
(Reporter)

Comment 50

5 years ago
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.
Attachment #621032 - Attachment description: Patch to replace assertJS v9.1 (mozilla-aurora) → Patch to replace assertJS v9.1 (mozilla-aurora) [checked-in]
Attachment #621032 - Flags: review?(hskupin) → review+
(Reporter)

Updated

5 years ago
Attachment #620747 - Attachment description: Patch to replace assertJS v9.1 (mozilla-beta) → Patch to replace assertJS v9.1 (mozilla-beta) [checked-in]
Attachment #620747 - Flags: review?(hskupin) → review+
(Reporter)

Updated

5 years ago
Attachment #620756 - Attachment description: Patch to replace assertJS v9.1 (mozilla-release) → Patch to replace assertJS v9.1 (mozilla-release) [checked-in]
Attachment #620756 - Flags: review?(hskupin) → review+
(Reporter)

Updated

5 years ago
Attachment #620757 - Attachment description: Patch to replace assertJS v9.1 (mozilla-esr10) → Patch to replace assertJS v9.1 (mozilla-esr10) [checked-in]
Attachment #620757 - Flags: review?(hskupin) → review+
(Reporter)

Comment 51

5 years ago
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)
status1.9.2: --- → wontfix
status-firefox-esr10: affected → fixed
status-firefox12: affected → fixed
status-firefox13: affected → fixed
status-firefox14: affected → fixed
Whiteboard: [lib][mozmill-refactor] → [lib][mozmill-refactor][qa-]
You need to log in before you can comment on or make changes to this bug.