782918 - Get rid of controller.assert() calls in favor of expect.* and assert.* methods

Status: RESOLVED FIXED

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

Description

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

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

Comment 1

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

Replaced all controller.assert with assert.* and expect.*

Comment 2

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

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

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

::: lib/search.js
@@ +771,5 @@
>      this._controller.click(engineManager);
>      md.waitForDialog();
> +
> +    assert.ok(!this.enginesDropDownOpen,
> +              "The search engine drop down menu has been closed");

This is just a check that the dropdown has been closed. I do not think that we should assert here.

::: tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ +58,5 @@
>    nodeCollector.root = controller.window.document.getElementById("PlacesToolbarItems");
>    var items = nodeCollector.queryNodes("toolbarbutton").elements;
>  
>    // For a default profile there should be exactly 2 items
> +  expect.equal(items.length, 2, "Bookmarks Toolbar contains 2 items");

This should probably be an assert because it would raise an exception in the next check if there is no element in the `items` array.

::: tests/functional/restartTests/testRestartChangeArchitecture/test1.js
@@ +29,5 @@
>  /**
>   * Verify that we're in 64 bit mode
>   */
>  function testArchitecture64bit() {
> +  expect.equal(runtime.XPCOMABI, "x86_64-gcc3", "ABI should be correct");

I would take the comment of the function as message parameter and get rid of the former.

::: tests/functional/restartTests/testRestartChangeArchitecture/test2.js
@@ +14,5 @@
>   */
>  function testRestartedNormally() {
>    var runtime = Cc["@mozilla.org/xre/runtime;1"].
>                  getService(Ci.nsIXULRuntime);
> +  expect.equal(runtime.XPCOMABI, "x86_64-gcc3", "ABI should be correct");

Same as for the last test.

::: tests/functional/restartTests/testRestartChangeArchitecture/test3.js
@@ +14,5 @@
>   */
>  function testRestarted32bit() {
>    var runtime = Cc["@mozilla.org/xre/runtime;1"].
>                  getService(Ci.nsIXULRuntime);
> +  expect.equal(runtime.XPCOMABI, "x86-gcc3", "ABI should be correct");

Same here.

::: tests/functional/restartTests/testRestartChangeArchitecture/test4.js
@@ +14,5 @@
>   */
>  function testArchitecture32bit() {
>    var runtime = Cc["@mozilla.org/xre/runtime;1"].
>                  getService(Ci.nsIXULRuntime);
> +  expect.equal(runtime.XPCOMABI, "x86-gcc3", "ABI should be correct");

Same here.

::: tests/functional/restartTests/testRestartChangeArchitecture/test5.js
@@ +14,5 @@
>   */
>  function testRestarted64bit() {
>    var runtime = Cc["@mozilla.org/xre/runtime;1"].
>                  getService(Ci.nsIXULRuntime);
> +  expect.equal(runtime.XPCOMABI, "x86_64-gcc3", "ABI should be correct");

Same here.

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +92,5 @@
>  
>    // Check that the underlined URL matches the entered URL
>    underlined.forEach(function (element) {
> +    expect.equal(element.toLowerCase(), LOCAL_TEST_PAGES[0].name,
> +                 "Underlined " + aType + " matches entered " + aType);

Please update the message so it only contains `aType` once.

::: tests/functional/testPreferences/testPasswordSavedAndDeleted.js
@@ +51,5 @@
>                   '/anon({"anonid":"button"})'
>                 );
>    var notification = locationBar.getNotification();
>    controller.waitThenClick(button);
>    

nit: white-space issue

::: tests/functional/testPreferences/testRestoreHomepageToDefault.js
@@ +50,5 @@
>    // Check that the current homepage is set to the default homepage - about:home
>    var currentHomepage = prefs.preferences.getPref("browser.startup.homepage", "");
>    var defaultHomepage = utils.getDefaultHomepage();
>  
> +  expect.equal(currentHomepage, defaultHomepage, "Default homepage restored");

I think we should make this an assert.

::: tests/functional/testSecurity/testDVCertificate.js
@@ +52,5 @@
>    var lockIcon = new elementslib.ID(controller.window.document, "identity-popup-encryption-icon");
>    var cssInfoLockImage = utils.getElementStyle(lockIcon, 'list-style-image');
>    var lockImageVisible = (cssInfoLockImage !== 'none');
>  
> +  expect.ok(lockImageVisible, "There is a lock icon");

There is no need anymore to declare `lockImageVisible`.  So use `expect.notEqual()` here.

::: tests/functional/testSecurity/testGreenLarry.js
@@ +67,5 @@
>    var lockIcon = new elementslib.ID(controller.window.document, "identity-popup-encryption-icon");
>    var cssInfoLockImage = utils.getElementStyle(lockIcon, 'list-style-image');
>    var lockImageVisible = (cssInfoLockImage !== 'none');
>  
> +  expect.ok(lockImageVisible, "There is a lock icon");

Same as for the DV cert test.

::: tests/functional/testSecurity/testSSLDisabledErrorPage.js
@@ +68,5 @@
>    var text = new elementslib.ID(controller.tabs.activeTab, "errorShortDescText");
>    controller.waitForElement(text, gTimeout);
>  
> +  expect.contain(text.getNode().textContent, 'ssl_error_ssl_disabled',
> +                 "The SSL error message contains 'ssl_error_ssl_disabled'");

No need to mention the state. Instead update the message so it tells what we really check here.

@@ +73,3 @@
>  
> +  expect.contain(text.getNode().textContent, 'mail.mozilla.org',
> +                 "The SSL error message contains 'mail.mozilla.org'");

Same here.

@@ +76,4 @@
>  
>    var PSMERR_SSL_Disabled = utils.getProperty(property, 'PSMERR_SSL_Disabled');
> +  expect.contain(text.getNode().textContent, PSMERR_SSL_Disabled,
> +                 "The SSL error message contains");

Same here, even that it looks not ready yet.

::: tests/functional/testTabView/testTabGroupNaming.js
@@ +25,5 @@
>    // Open Tab Groups View (default via keyboard shortcut)
>    activeTabView.open();
>  
>    // Verify that one tab group exists
> +  expect.equal(activeTabView.getGroups().length, 1, "One tab group exists");

This has to be an assert and kill the comment.

@@ +36,5 @@
>    controller.type(title, TABGROUP_TITLE);
>  
>    // Verify that the tab group has a new name
> +  expect.equal(title.getNode().value, TABGROUP_TITLE,
> +               "Tab group title has been set");

No need for the above comment anymore.

@@ +47,5 @@
>  
>     // Verify that the tab group has retained its new name
> +   
> +  expect.equal(title.getNode().value, TABGROUP_TITLE,
> +               "Tab group title has been set");

Same here, even it looks like the message is wrong.

::: tests/functional/testTabView/testToggleTabView.js
@@ +22,5 @@
>    // Open Tab View (default via keyboard shortcut)
>    activeTabView.open();
>  
>    // Check that Tab View has opened 
> +  expect.ok(activeTabView.isOpen, "Tab View has opened");

Kill the comment and use `has been`. Also it really looks like it should be an assert.

@@ +28,5 @@
>    // Close Tab View (default via keyboard shortcut)
>    activeTabView.close();
>  
>    // Check that Tab View has closed
> +  expect.ok(!activeTabView.isOpen, "Tab View has closed");

Same here.

::: tests/functional/testTabbedBrowsing/testTabPinning.js
@@ +33,5 @@
>    contextMenu.select("#context_pinTab", currentTab);
>  
>    // check whether it's sucessfully pinned  
>    var appTabPinned = tabBrowser.isAppTab(currentTab);
> +  assert.ok(appTabPinned, "This tab has been pinned");

Kill the comment and update the message to `Current tab...`

@@ +39,5 @@
>    contextMenu.select("#context_unpinTab", currentTab);
>  
>    // check whether it's successfully unpinned  
>    var appTabUnpinned = !tabBrowser.isAppTab(currentTab);
> +  assert.ok(appTabUnpinned, "The tab is unpinned");

Same as above.

::: tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test1.js
@@ +50,5 @@
>    var addToFirefox = discovery.getElement({type: "addon_installButton"});
>    var currentInstallSource = discovery.getInstallSource(addToFirefox);
>  
> +  expect.equal(currentInstallSource, INSTALL_SOURCE,
> +               "Installation link has source set");

Without the right element we will fail. So this has to be an assert.

::: tests/remote/restartTests/testDiscoveryPane_FirstTimeModule/test1.js
@@ +53,5 @@
>    // Install the addon
>    var addToFirefox = discovery.getElement({type: "addon_installButton"});
>    var currentInstallSource = discovery.getInstallSource(addToFirefox);
>  
> +  expect.equal(currentInstallSource, INSTALL_SOURCE,

Same as for the former test.

::: tests/remote/restartTests/testDiscoveryPane_installCollectionAddon/test1.js
@@ +57,5 @@
>    // Retrieve addon src parameter from installation link
>    var currentInstallSource = discovery.getInstallSource(addToFirefox);
>  
> +  expect.equal(currentInstallSource, INSTALL_SOURCE,
> +               "Installation link has source set");

Same as for the former test.

@@ +70,5 @@
>    am.setCategory({category: am.getCategoryById({id: "extension"})});
>   
>    var addon = am.getAddons({attribute: "value", value: addonId})[0];
>  
> +  assert.ok(am.isAddonInstalled({addon: addon}), "Add-on has been installed");

In the other tests we have:

`var addonIsInstalled = am.isAddonInstalled({addon: addon});`

I think that one is better and we should update the other tests.

::: tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js
@@ +56,5 @@
>    // Retrieve addon src parameter from installation link
>    var currentInstallSource = discovery.getInstallSource(addToFirefox);
>  
> +  expect.equal(currentInstallSource, INSTALL_SOURCE,
> +               "Installation link has source set");

Same as for the former test.

Comment 3

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Comment on attachment 659163 [details] [diff] [review]
> ::: tests/functional/testPreferences/testPasswordSavedAndDeleted.js
> @@ +51,5 @@
> >                   '/anon({"anonid":"button"})'
> >                 );
> >    var notification = locationBar.getNotification();
> >    controller.waitThenClick(button);
> >    
> 
> nit: white-space issue
That was not an issue introduced by my patch, but context code. It was anyway fixed by bug 789030.
In the future, to see only whitespaces introduced by the patch, run this command in the patch directory:
> grep -r "^+.* $" ./

Comment 4

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

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

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

::: tests/functional/restartTests/testRestartChangeArchitecture/test1.js
@@ +29,5 @@
>  /**
>   * Verify that we're in 64 bit mode
>   */
>  function testArchitecture64bit() {
> +  expect.equal(runtime.XPCOMABI, "x86_64-gcc3", "Verify that we're in 64 bit mode");

As mentioned in my last review please get rid of the block comment. This applies to all of the architecture tests.

Preferable I would use a message like: "By default the application launches in 64bit mode". Message for the other tests should refer to the restart mode.

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Include required modules
> +var {expect} = require("../../../lib/assertions");

I had to backout the change on bug 790174. So please coordinate with Andreea how to land. I would say we take the change on this bug first.

::: tests/functional/testTabView/testToggleTabView.js
@@ +27,5 @@
>    // Close Tab View (default via keyboard shortcut)
>    activeTabView.close();
>  
>    // Check that Tab View has closed
> +  assert.ok(!activeTabView.isOpen, "Tab View has been closed");

There is still a useless comment around.

::: tests/functional/testTabbedBrowsing/testTabPinning.js
@@ +40,2 @@
>    var appTabUnpinned = !tabBrowser.isAppTab(currentTab);
> +  assert.ok(appTabUnpinned, "Current tab is unpinned");

nit: similar as above 'has been...'

::: tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test1.js
@@ +50,5 @@
>    var addToFirefox = discovery.getElement({type: "addon_installButton"});
>    var currentInstallSource = discovery.getInstallSource(addToFirefox);
>  
> +  expect.equal(currentInstallSource, INSTALL_SOURCE,
> +               "Installation link has source set");

Please ensure while doing the internal review that all review comments have been addressed. Again, this should be an assert.

::: tests/remote/restartTests/testDiscoveryPane_installCollectionAddon/test1.js
@@ +74,2 @@
>  
> +  assert.ok(addonIsInstalled, "Add-on has been installed");

You have made this change the other way around. As mentioned in my last review we should update the other tests while this one was fine before.

Comment 5

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v3.0

addressed all comments

Comment 6

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

Why is it only me who have been asked for review? Please be more sensitive in asking reviews in the future.

Comment 7

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Why is it only me who have been asked for review? Please be more sensitive
> in asking reviews in the future.
If you'll look at history(https://bugzilla.mozilla.org/show_activity.cgi?id=782918) you can see the first review was requested to both you and Dave. Since you took the first review, it is only natural to take all further reviews, since I was addressing your comments. If we ask for both of your reviews on each iterations, it will only increase the confusion and will lengthen the review cycle.

Comment 8

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

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

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

Please don't do any patch upload or fix in a hurry. Do a wisely internal review as what we want to see.

(In reply to Henrik Skupin (:whimboo) from comment #4)
> >  /**
> >   * Verify that we're in 64 bit mode
> >   */
> >  function testArchitecture64bit() {
> > +  expect.equal(runtime.XPCOMABI, "x86_64-gcc3", "Verify that we're in 64 bit mode");
> 
> As mentioned in my last review please get rid of the block comment. This
> applies to all of the architecture tests.
> 
> Preferable I would use a message like: "By default the application launches
> in 64bit mode". Message for the other tests should refer to the restart mode.

The latest change on those files have been completely misunderstood. Please read my comments closely and do a sensitive internal review before uploading patches. 

> ::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
> @@ +2,5 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  // Include required modules
> > +var {expect} = require("../../../lib/assertions");
> 
> I had to backout the change on bug 790174. So please coordinate with Andreea
> how to land. I would say we take the change on this bug first.

So what's the status here? I really would encourage you to give feedback on any review comment I made. I don't understand how you want to proceed here.

> ::: tests/functional/testTabbedBrowsing/testTabPinning.js
> @@ +40,2 @@
> >    var appTabUnpinned = !tabBrowser.isAppTab(currentTab);
> > +  assert.ok(appTabUnpinned, "Current tab is unpinned");
> 
> nit: similar as above 'has been...'

Not done.

Comment 9

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

(In reply to Alex Lakatos[:AlexLakatos] from comment #7)
> If you'll look at
> history(https://bugzilla.mozilla.org/show_activity.cgi?id=782918) you can
> see the first review was requested to both you and Dave. Since you took the
> first review, it is only natural to take all further reviews, since I was
> addressing your comments. If we ask for both of your reviews on each
> iterations, it will only increase the confusion and will lengthen the review
> cycle.

This is not true. And I'm not sure why you are making such an assumption. Please simply follow the guidelines we have setup for you all.

Comment 10

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> > I had to backout the change on bug 790174. So please coordinate with Andreea
> > how to land. I would say we take the change on this bug first.
> 
> So what's the status here? I really would encourage you to give feedback on
> any review comment I made. I don't understand how you want to proceed here.
At first we agreed we will wait for this patch to land first. But since we had the failure on bug 790174 and it was needed an update on testCheckItemsHighlight.js too, that one got landed first, so we could get results there over the weekend.

Comment 11

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Comment on attachment 660820 [details] [diff] [review]
> (In reply to Henrik Skupin (:whimboo) from comment #4)
> > >  /**
> > >   * Verify that we're in 64 bit mode
> > >   */
> > >  function testArchitecture64bit() {
> > > +  expect.equal(runtime.XPCOMABI, "x86_64-gcc3", "Verify that we're in 64 bit mode");
> > 
> > As mentioned in my last review please get rid of the block comment. This
> > applies to all of the architecture tests.
> > 
> > Preferable I would use a message like: "By default the application launches
> > in 64bit mode". Message for the other tests should refer to the restart mode.
> 
> The latest change on those files have been completely misunderstood. Please
> read my comments closely and do a sensitive internal review before uploading
> patches. 
I'm not comfortable removing the comment block. Our style guide says "All functions should be preceded by a comment block using the JSDoc Toolkit style". https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Commenting
Is there any particular reason why we are not following the style guide and are removing the comments just on these architecture tests?

Comment 12

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

we have to revise the coding style a bit but for now you are right so leave them in with a good comment.

Comment 13

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.0

passed internal review. left comments on the architecture tests unchanged, as discussed with Dave on irc.

Comment 14

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

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

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

This collides again with Andreea's patch on bug 792394. I thought that you both have synched up so it doesn't happen again.

::: tests/functional/restartTests/testRestartChangeArchitecture/test2.js
@@ +14,5 @@
>   */
>  function testRestartedNormally() {
>    var runtime = Cc["@mozilla.org/xre/runtime;1"].
>                  getService(Ci.nsIXULRuntime);
> +  expect.equal(runtime.XPCOMABI, "x86_64-gcc3", "By default the application launches in 64bit mode");

Why by default? It should be 'after a restart' or something like that.

::: tests/functional/restartTests/testRestartChangeArchitecture/test3.js
@@ +14,5 @@
>   */
>  function testRestarted32bit() {
>    var runtime = Cc["@mozilla.org/xre/runtime;1"].
>                  getService(Ci.nsIXULRuntime);
> +  expect.equal(runtime.XPCOMABI, "x86-gcc3", "By default the application launches in 32bit mode");

Same here. We restarted in 32bit mode. That should be clear when reading the message.

::: tests/functional/restartTests/testRestartChangeArchitecture/test4.js
@@ +14,5 @@
>   */
>  function testArchitecture32bit() {
>    var runtime = Cc["@mozilla.org/xre/runtime;1"].
>                  getService(Ci.nsIXULRuntime);
> +  expect.equal(runtime.XPCOMABI, "x86-gcc3", "By default the application launches in 32bit mode");

Same here.

::: tests/functional/restartTests/testRestartChangeArchitecture/test5.js
@@ +14,5 @@
>   */
>  function testRestarted64bit() {
>    var runtime = Cc["@mozilla.org/xre/runtime;1"].
>                  getService(Ci.nsIXULRuntime);
> +  expect.equal(runtime.XPCOMABI, "x86_64-gcc3", "By default the application launches in 64bit mode");

... and here.

Comment 15

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v5.0

>This collides again with Andreea's patch on bug 792394. I thought that you both 
>have synched up so it doesn't happen again.
Synced now. This patch does not touch that file anymore.
Changed comments on architecture tests.
Fixed import failures due to the recent lands in our repo.

Comment 16

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

Comment on attachment 664565 [details] [diff] [review]
patch v5.0

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

Just one note because I don't want to cycle through review requests for assertion messages more than 1 time in the future. Please think intensely about the wording. It should explain what we are testing in the given state. If it's not clear to you what to use please ask on IRC. I hope the next iteration will be the final one.

::: tests/functional/restartTests/testRestartChangeArchitecture/test3.js
@@ +15,5 @@
>  function testRestarted32bit() {
>    var runtime = Cc["@mozilla.org/xre/runtime;1"].
>                  getService(Ci.nsIXULRuntime);
> +  expect.equal(runtime.XPCOMABI, "x86-gcc3",
> +               "After a restart the application launches in 32bit mode");

This is not a normal restart. So it should explicitly called out here.

::: tests/functional/restartTests/testRestartChangeArchitecture/test4.js
@@ +15,5 @@
>  function testArchitecture32bit() {
>    var runtime = Cc["@mozilla.org/xre/runtime;1"].
>                  getService(Ci.nsIXULRuntime);
> +  expect.equal(runtime.XPCOMABI, "x86-gcc3",
> +               "After a restart the application launches in 32bit mode");

"launches still in..."

::: tests/functional/restartTests/testRestartChangeArchitecture/test5.js
@@ +15,5 @@
>  function testRestarted64bit() {
>    var runtime = Cc["@mozilla.org/xre/runtime;1"].
>                  getService(Ci.nsIXULRuntime);
> +  expect.equal(runtime.XPCOMABI, "x86_64-gcc3",
> +               "After a restart the application launches in 64bit mode");

Again, this is not a normal restart.

Comment 17

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v6.0

edited the messages for testRestartChangeArchitecture, as instructed in comment 16, after checking with Henrik on irc

Comment 18

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

Comment on attachment 673230 [details] [diff] [review]
patch v6.0

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

Looks fine now. Will land it right away. Please watch out for regressions over the weekend. If everything is fine we will backport on Monday.

Comment 19

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

http://hg.mozilla.org/qa/mozmill-tests/rev/446d942450d1 (default)

Comment 20

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> Comment on attachment 673230 [details] [diff] [review]
> patch v6.0
> 
> Review of attachment 673230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine now. Will land it right away. Please watch out for regressions
> over the weekend. If everything is fine we will backport on Monday.
No new bugs have been logged over the weekend. I'll start backporting.

Comment 21

[Alex Lakatos[:AlexLakatos]]

Attachment: backport patch [aurora, beta, release]

backport patch for aurora, beta and release

Comment 22

[Alex Lakatos[:AlexLakatos]]

Attachment: backport patch [esr]

backport patch for the esr branch

Comment 23

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

Comment on attachment 674233 [details] [diff] [review]
backport patch [aurora, beta, release]

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

Looks fine but I will unset the review request for the backport, because I have seen that we still have some controller.assert() calls alive in default for the Selenium addon tests. Those we will have to change on default first and combine them in single backport patches. Sorry for missing that before.

Comment 24

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

Comment on attachment 674238 [details] [diff] [review]
backport patch [esr]

Same as for the other backport patch.

Comment 25

[Alex Lakatos[:AlexLakatos]]

I was under the impression that addon tests are not maintained by us, but by the addon owners. That's why I left them alone. But I'll add a patch for default asap.

Comment 26

[Alex Lakatos[:AlexLakatos]]

Attachment: seleniumtests patch v1.0

refactored selenuim tests on default branch

Comment 27

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

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

Dave should review it

Comment 28

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

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

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

Please use first initial surname for reviewers, so I would be 'dhunt' rather than 'davehunt'.

::: tests/addons/[e-mail]/lib/checks.js
@@ +18,5 @@
>   // XXX: Bug 621214 - Find a way to check properties of treeView rows
>  
>   //check suite progress indicator
>   var isSuiteProgressIndicatorGreen = seleniumManager.isSuiteProgressIndicatorGreen;
> + assert.ok(isSuiteProgressIndicatorGreen, "Suite progress indicator is green");

I would use an expect here rather than an assert.

@@ +40,5 @@
>    // XXX: Bug 621214 - Find a way to check properties of treeView rows
>  
>    //check suite progress indicator
>    var isSuiteProgressIndicatorRed = seleniumManager.isSuiteProgressIndicatorRed;
> +  assert.ok(isSuiteProgressIndicatorRed, "Suite progress indicator is red");

As above, I would suggest using expect rather than assert here.

Comment 29

[Alex Lakatos[:AlexLakatos]]

Attachment: seleniumtests patch v2.0

If this is ok, I'll upload the combined backports

Comment 30

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

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

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

Looks great! Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/1bc78543a261 (default)

We can transplant/backport once we can verify the CI results.

Comment 31

[Alex Lakatos[:AlexLakatos]]

Attachment: combined backport patch [aurora, beta, release]

combined backport

Comment 32

[Alex Lakatos[:AlexLakatos]]

Attachment: combined backport patch [esr]

combined backport

Comment 33

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

Comment on attachment 677481 [details] [diff] [review]
combined backport patch [aurora, beta, release]

The change in tests/functional/testSecurity/testDVCertificate.js has caused a regression that we need to fix with a follow-up patch before we can land a combined backport.

-  controller.assert(function () {
-    return favicon.getNode().getAttribute("hidden") == false;
-  }, "Lock icon is visible in identity box");
+  expect.equal(favicon.getNode().getAttribute("hidden"), "false",
+               "Lock icon is visible in identity box");

This passes if it's changed to:

expect.ok(!favicon.getNode().getAttribute("hidden") == false);

However a better proposal might be:

expect.ok(!favicon.getNode().hasAttribute("hidden"));

I suspect this went unnoticed as this test was recently disabled.

Comment 34

[Andreea Matei [:AndreeaMatei]]

Attachment: follow-up v1 [default]

Replacing expect.equal() with expect.ok() for the favicon check.

Comment 35

[Andreea Matei [:AndreeaMatei]]

Attachment: follow-up v1.1

Updated the favicon check and added one for the "identity-icons-https.png" image as discussed on IRC.
There's another one in the test, for the lock image in the identity box, which was compared to "none" so I updated that also.

Comment 36

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

Comment on attachment 678710 [details] [diff] [review]
follow-up v1.1

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

Please change the commit message to be relevant to the changes in the patch.

::: tests/functional/testSecurity/testDVCertificate.js
@@ +31,5 @@
>    var favicon = new elementslib.ID(controller.window.document, "page-proxy-favicon");
> +  expect.ok(!favicon.getNode().hasAttribute("hidden"),
> +            "Lock icon is visible in identity box");
> +
> +  var cssLockImage = utils.getElementStyle(favicon, 'list-style-image');

I would call this faviconImage.

Comment 37

[Andreea Matei [:AndreeaMatei]]

Attachment: lock icon v2

Updated the message and the image name variable.

Comment 38

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

Comment on attachment 678743 [details] [diff] [review]
lock icon v2

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

::: tests/functional/testSecurity/testDVCertificate.js
@@ +54,5 @@
>    // Check for the Lock icon is visible
>    var lockIcon = new elementslib.ID(controller.window.document, "identity-popup-encryption-icon");
>    var cssInfoLockImage = utils.getElementStyle(lockIcon, 'list-style-image');
>  
> +  expect.contain(cssInfoLockImage, "Secure.png", "There is a lock icon");

This line is failing for me. The image appears to be "chrome://browser/skin/Secure-Glyph-White.png"

This might be dependant on the platform (I'm running Mac OS X). Perhaps we could use match and a regular expression here. Please test on all platforms wherever possible.

Comment 39

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

(In reply to Dave Hunt (:davehunt) from comment #38)
> This line is failing for me. The image appears to be
> "chrome://browser/skin/Secure-Glyph-White.png"
> 
> This might be dependant on the platform (I'm running Mac OS X). Perhaps we
> could use match and a regular expression here. Please test on all platforms
> wherever possible.

Or would it be enough to check that it is not empty? Sounds easier and should have the same effect.

Comment 40

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

Comment on attachment 678743 [details] [diff] [review]
lock icon v2

Please use better names for attachments for follow-up patches.

Comment 41

[Andreea Matei [:AndreeaMatei]]

I checked the image and it's different indeed on  each platform. The value when there is no image is "none", so I will leave the expect as it was (checking it's no empty).
The one for favicon is not different.

Comment 42

[Andreea Matei [:AndreeaMatei]]

Attachment: lock icon v2.1

Adding the favicon lock check only.

Comment 43

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

Comment on attachment 679121 [details] [diff] [review]
lock icon v2.1

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/96a3d438ae1b (default)

Now we need an update for the combined backport.

Comment 44

[Andreea Matei [:AndreeaMatei]]

Attachment: [beta, release, aurora] backport

Combined backport for beta, release and aurora.

Comment 45

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

Comment on attachment 679138 [details] [diff] [review]
[beta, release, aurora] backport

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

Included all patches correctly. No instance of controller.assert() is left. Lets get this in tomorrow after the next testrun for 19.0a1 builds.

Comment 46

[Andreea Matei [:AndreeaMatei]]

Attachment: [esr] backport

The last one for esr10.

Comment 47

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

Comment on attachment 679121 [details] [diff] [review]
lock icon v2.1

>+++ b/tests/functional/testSecurity/testDVCertificate.js
>   // Check the favicon
>   var favicon = new elementslib.ID(controller.window.document, "page-proxy-favicon");
>-  expect.equal(favicon.getNode().getAttribute("hidden"), "false",
>-               "Lock icon is visible in identity box");
>+  expect.ok(!favicon.getNode().hasAttribute("hidden"),
>+            "Lock icon is visible in identity box");
>+
>+  var faviconImage = utils.getElementStyle(favicon, 'list-style-image');
>+  expect.contain(faviconImage, "identity-icons-https.png", "There is a lock icon");

Why has this test not been enabled with this patch? It's still disabled on default:
http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testSecurity/testDVCertificate.js

I thought we are doing everything in that bug. But to not add another review churn here I will go ahead and backout the skip patch from bug 804374. In the future make sure you really run the test it would have caught this problem. I don't want to see changes to files we actually can't test on CI because it's disabled.

Comment 48

[Andreea Matei [:AndreeaMatei]]

Sorry, I understood differently regarding the skip, that will be solved in bug 804374.

Comment 49

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

I triggered a testrun across platforms for Nightly so we can ensure all is fine. Once finished and it passes I will go ahead and land stuff on the other branches.

Comment 50

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

All looks fine. So I'm going to land the remaining patches now.

Comment 51

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

http://hg.mozilla.org/qa/mozmill-tests/rev/fed4d9a20ca7 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/1cb02a8c651f (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/251edd9cc96a (release)

Comment 52

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

Comment on attachment 679148 [details] [diff] [review]
[esr] backport

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

Comment 53

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

Yay! We are done on that. Great to see that controller.assert() is finally gone now.

Comment 54

[Andreea Matei [:AndreeaMatei]]

It seems on esr we don't have any favicon image for the page from testDVCertificate.js:

http://mozmill-ci.blargon7.com/#/functional/failure?branch=All&platform=All&from=2012-11-10&to=&test=%2FtestSecurity%2FtestDVCertificate.js&func=testDVCertificate.js%3A%3AtestLarryBlue

Do we want to check it's none or just remove the favicon image check?

Comment 55

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #54)
> It seems on esr we don't have any favicon image for the page from
> testDVCertificate.js:

Not anymore. I have uploaded a global favicon for the site which is the mozilla.ico file in the root folder. 

> Do we want to check it's none or just remove the favicon image check?

Revert the code to what we had before with the updated favicon url. That should fix it.

Comment 56

[Andreea Matei [:AndreeaMatei]]

Attachment: [esr] Revert to favicon for DV cert test v1

Backing out the changes for testDVCertificate.js
The controller.waitFor() will be replaced with assert/expect in bug 793092, the patch there is in internal reviews.

Comment 57

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

Comment on attachment 680572 [details] [diff] [review]
[esr] Revert to favicon for DV cert test v1

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

Comment 58

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

Thanks for the quick turnaround.  Should be all fixed now.

Comment 59

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

The esr10 backport caused bug 810770. This should have been picked up when testing the backport. Please provide a follow-up patch for esr10.

Comment 60

[Daniela Petrovici]

Attachment: [esr] testPasswordSavedAndDeleted patch v1.0

fix for failing testPasswordSavedAndDeleted on esr10

Comment 61

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

Comment on attachment 682425 [details] [diff] [review]
[esr] testPasswordSavedAndDeleted patch v1.0

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

Next time please provide a commit message which describes what this patch is for. Using the bug summary is not helpful at all here, given that multiple patches are on this bug. Thanks.

http://hg.mozilla.org/qa/mozmill-tests/rev/3d9b839bf97e (esr10)

Comment 62

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

Should be fixed now. Next time please also test the patch on the wanted target branch. It's not enough to check that it applies correctly.