The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: whimboo, Assigned: AndreeaMatei)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox16 fixed, firefox17 fixed, firefox18 fixed, firefox19 fixed, firefox-esr10 fixed)

Details

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

Attachments

(7 attachments, 13 obsolete attachments)

48.02 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
3.62 KB, patch
davehunt
: review+
Details | Diff | Splinter Review
1.58 KB, patch
davehunt
: review+
davehunt
: checkin+
Details | Diff | Splinter Review
51.55 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
51.53 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
1.60 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
1.43 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
Assignee: nobody → alex.lakatos
Status: NEW → ASSIGNED
(Reporter)

Updated

5 years ago
Blocks: 744007
(Reporter)

Updated

5 years ago
Whiteboard: s=2012-8-27 u=enhancement c=mozmill
(Reporter)

Updated

5 years ago
Whiteboard: s=2012-8-27 u=enhancement c=mozmill → s=2012-8-27 u=enhancement c=assertions

Updated

5 years ago
Whiteboard: s=2012-8-27 u=enhancement c=assertions → s=2012-8-27 u=enhancement c=assertions p=1
Created attachment 659163 [details] [diff] [review]
patch v1.0

Replaced all controller.assert with assert.* and expect.*
Attachment #659163 - Flags: review?(hskupin)
Attachment #659163 - Flags: review?(dave.hunt)
(Reporter)

Comment 2

5 years ago
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.
Attachment #659163 - Flags: review?(hskupin)
Attachment #659163 - Flags: review?(dave.hunt)
Attachment #659163 - Flags: review-
(Reporter)

Updated

5 years ago
Whiteboard: s=2012-8-27 u=enhancement c=assertions p=1 → s=q3 u=enhancement c=assertions p=1
Created attachment 660446 [details] [diff] [review]
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 "^+.* $" ./
Attachment #659163 - Attachment is obsolete: true
Attachment #660446 - Flags: review?(hskupin)
(Reporter)

Comment 4

5 years ago
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.
Attachment #660446 - Flags: review?(hskupin) → review-
Created attachment 660820 [details] [diff] [review]
patch v3.0

addressed all comments
Attachment #660446 - Attachment is obsolete: true
Attachment #660820 - Flags: review?(hskupin)
(Reporter)

Comment 6

5 years ago
Why is it only me who have been asked for review? Please be more sensitive in asking reviews in the future.
(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.
(Reporter)

Comment 8

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

Comment 9

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

Comment 10

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

Updated

5 years ago
Blocks: 787924
(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?
(Assignee)

Updated

5 years ago
No longer blocks: 787924
(Reporter)

Comment 12

5 years ago
we have to revise the coding style a bit but for now you are right so leave them in with a good comment.
(Assignee)

Updated

5 years ago
Blocks: 792394
(Reporter)

Updated

5 years ago
No longer blocks: 792394
Created attachment 663017 [details] [diff] [review]
patch v4.0

passed internal review. left comments on the architecture tests unchanged, as discussed with Dave on irc.
Attachment #660820 - Attachment is obsolete: true
Attachment #663017 - Flags: review?(hskupin)
Attachment #663017 - Flags: review?(dave.hunt)
(Reporter)

Comment 14

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

Comment 16

5 years ago
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.
Attachment #664565 - Flags: review?(hskupin)
Attachment #664565 - Flags: review?(dave.hunt)
Attachment #664565 - Flags: review-
(Reporter)

Updated

5 years ago
Whiteboard: s=q3 u=enhancement c=assertions p=1 → s=121008 u=enhancement c=assertions p=1
Created attachment 673230 [details] [diff] [review]
patch v6.0

edited the messages for testRestartChangeArchitecture, as instructed in comment 16, after checking with Henrik on irc
Attachment #664565 - Attachment is obsolete: true
Attachment #673230 - Flags: review?(hskupin)
Attachment #673230 - Flags: review?(dave.hunt)
(Reporter)

Comment 18

5 years ago
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.
Attachment #673230 - Flags: review?(hskupin)
Attachment #673230 - Flags: review?(dave.hunt)
Attachment #673230 - Flags: review+
(Reporter)

Comment 19

5 years ago
http://hg.mozilla.org/qa/mozmill-tests/rev/446d942450d1 (default)
status-firefox-esr10: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → fixed
(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.
Created attachment 674233 [details] [diff] [review]
backport patch [aurora, beta, release]

backport patch for aurora, beta and release
Attachment #674233 - Flags: review?(hskupin)
Attachment #674233 - Flags: review?(dave.hunt)
Created attachment 674238 [details] [diff] [review]
backport patch [esr]

backport patch for the esr branch
Attachment #674238 - Flags: review?(hskupin)
Attachment #674238 - Flags: review?(dave.hunt)
(Reporter)

Comment 23

5 years ago
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.
Attachment #674233 - Flags: review?(hskupin)
Attachment #674233 - Flags: review?(dave.hunt)
(Reporter)

Comment 24

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

Same as for the other backport patch.
Attachment #674238 - Flags: review?(hskupin)
Attachment #674238 - Flags: review?(dave.hunt)
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.
Created attachment 675168 [details] [diff] [review]
seleniumtests patch v1.0

refactored selenuim tests on default branch
Attachment #675168 - Flags: review?(hskupin)
Attachment #675168 - Flags: review?(dave.hunt)
(Reporter)

Comment 27

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

Dave should review it
Attachment #675168 - Flags: review?(hskupin)
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/ide@seleniumhq.org/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.
Attachment #675168 - Flags: review?(dave.hunt) → review-
Created attachment 676940 [details] [diff] [review]
seleniumtests patch v2.0

If this is ok, I'll upload the combined backports
Attachment #675168 - Attachment is obsolete: true
Attachment #676940 - Flags: review?(hskupin)
Attachment #676940 - Flags: review?(dave.hunt)
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.
Attachment #676940 - Flags: review?(hskupin)
Attachment #676940 - Flags: review?(dave.hunt)
Attachment #676940 - Flags: review+
Created attachment 677481 [details] [diff] [review]
combined backport patch [aurora, beta, release]

combined backport
Attachment #674233 - Attachment is obsolete: true
Attachment #677481 - Flags: review?(hskupin)
Attachment #677481 - Flags: review?(dave.hunt)
Created attachment 677482 [details] [diff] [review]
combined backport patch [esr]

combined backport
Attachment #674238 - Attachment is obsolete: true
Attachment #677482 - Flags: review?(hskupin)
Attachment #677482 - Flags: review?(dave.hunt)

Updated

5 years ago
Blocks: 804374
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.
Attachment #677481 - Flags: review?(hskupin)
Attachment #677481 - Flags: review?(dave.hunt)
Attachment #677481 - Flags: review-

Updated

5 years ago
Attachment #677482 - Flags: review?(hskupin)
Attachment #677482 - Flags: review?(dave.hunt)
Attachment #677482 - Flags: review-
(Assignee)

Updated

5 years ago
Assignee: alex.lakatos → andreea.matei
(Assignee)

Comment 34

5 years ago
Created attachment 678668 [details] [diff] [review]
follow-up v1 [default]

Replacing expect.equal() with expect.ok() for the favicon check.
Attachment #678668 - Flags: review?(hskupin)
Attachment #678668 - Flags: review?(dave.hunt)
(Assignee)

Updated

5 years ago
Attachment #678668 - Flags: review?(hskupin)
Attachment #678668 - Flags: review?(dave.hunt)
(Assignee)

Comment 35

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

Comment 37

5 years ago
Created attachment 678743 [details] [diff] [review]
lock icon v2

Updated the message and the image name variable.
Attachment #678710 - Attachment is obsolete: true
Attachment #678743 - Flags: review?(hskupin)
Attachment #678743 - Flags: review?(dave.hunt)
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.
Attachment #678743 - Flags: review?(hskupin)
Attachment #678743 - Flags: review?(dave.hunt)
Attachment #678743 - Flags: review-
(Reporter)

Comment 39

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

Updated

4 years ago
Attachment #677481 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #677482 - Attachment is obsolete: true
(Reporter)

Comment 40

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

Please use better names for attachments for follow-up patches.
Attachment #678743 - Attachment description: follow-up v2 → lock icon v2
(Assignee)

Comment 41

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

Comment 42

4 years ago
Created attachment 679121 [details] [diff] [review]
lock icon v2.1

Adding the favicon lock check only.
Attachment #678743 - Attachment is obsolete: true
Attachment #679121 - Flags: review?(hskupin)
Attachment #679121 - Flags: review?(dave.hunt)
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.
Attachment #679121 - Flags: review?(hskupin)
Attachment #679121 - Flags: review?(dave.hunt)
Attachment #679121 - Flags: review+
Attachment #679121 - Flags: checkin+
(Assignee)

Comment 44

4 years ago
Created attachment 679138 [details] [diff] [review]
[beta, release, aurora] backport

Combined backport for beta, release and aurora.
Attachment #679138 - Flags: review?(hskupin)
Attachment #679138 - Flags: review?(dave.hunt)
(Reporter)

Comment 45

4 years ago
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.
Attachment #679138 - Flags: review?(hskupin)
Attachment #679138 - Flags: review?(dave.hunt)
Attachment #679138 - Flags: review+
(Assignee)

Comment 46

4 years ago
Created attachment 679148 [details] [diff] [review]
[esr] backport

The last one for esr10.
Attachment #679148 - Flags: review?(hskupin)
Attachment #679148 - Flags: review?(dave.hunt)
(Reporter)

Comment 47

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

Comment 48

4 years ago
Sorry, I understood differently regarding the skip, that will be solved in bug 804374.
(Reporter)

Comment 49

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

Comment 50

4 years ago
All looks fine. So I'm going to land the remaining patches now.
(Reporter)

Comment 51

4 years ago
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)
status-firefox16: affected → fixed
status-firefox17: affected → fixed
status-firefox18: affected → fixed
(Reporter)

Updated

4 years ago
Attachment #679138 - Flags: checkin+
(Reporter)

Updated

4 years ago
Attachment #679148 - Flags: review?(hskupin)
Attachment #679148 - Flags: review?(dave.hunt)
Attachment #679148 - Flags: review+
(Reporter)

Comment 52

4 years ago
Comment on attachment 679148 [details] [diff] [review]
[esr] backport

http://hg.mozilla.org/qa/mozmill-tests/rev/ee76598e838b (esr10)
Attachment #679148 - Flags: checkin+
(Reporter)

Comment 53

4 years ago
Yay! We are done on that. Great to see that controller.assert() is finally gone now.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox-esr10: affected → fixed
Resolution: --- → FIXED
(Assignee)

Comment 54

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

Comment 55

4 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 56

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

Updated

4 years ago
Attachment #680572 - Flags: review?(hskupin)
Attachment #680572 - Flags: review?(dave.hunt)
Attachment #680572 - Flags: review+
(Reporter)

Comment 57

4 years ago
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)
Attachment #680572 - Attachment description: [esr] Favicon image v1 → [esr] Revert to favicon for DV cert test v1
Attachment #680572 - Flags: checkin+
(Reporter)

Comment 58

4 years ago
Thanks for the quick turnaround.  Should be all fixed now.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
The esr10 backport caused bug 810770. This should have been picked up when testing the backport. Please provide a follow-up patch for esr10.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 60

4 years ago
Created attachment 682425 [details] [diff] [review]
[esr] testPasswordSavedAndDeleted patch v1.0

fix for failing testPasswordSavedAndDeleted on esr10
Attachment #682425 - Flags: review?(hskupin)
Attachment #682425 - Flags: review?(dave.hunt)

Updated

4 years ago
Blocks: 810770
(Reporter)

Comment 61

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

Comment 62

4 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.