803088 - Get rid of controller.assertText() and controller.assertValue() in favor of expect.* and assert.* methods

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

Similar to bug 789916 we have to get rid of all the instances of controller.assertText() and controller.assertValue() and make use of the assertions module instead.

Comment 1

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

repaced assertValue and assertText, using getNode().value and getNode().textContent

Comment 2

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

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

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

::: tests/addons/[e-mail]/lib/checks.js
@@ +21,5 @@
>   var isSuiteProgressIndicatorGreen = seleniumManager.isSuiteProgressIndicatorGreen;
>   expect.ok(isSuiteProgressIndicatorGreen, "Suite progress indicator is green");
>  
>   //check suite counts
> + expect.equal(seleniumManager.runCount.getNode().value, "1", "Run count is '1'");

Please change message to "1 test has run".

@@ +22,5 @@
>   expect.ok(isSuiteProgressIndicatorGreen, "Suite progress indicator is green");
>  
>   //check suite counts
> + expect.equal(seleniumManager.runCount.getNode().value, "1", "Run count is '1'");
> + expect.equal(seleniumManager.failureCount.getNode().value, "0", "Failure count is '0'");

Please change message to "No tests have failed".

@@ +43,5 @@
>    var isSuiteProgressIndicatorRed = seleniumManager.isSuiteProgressIndicatorRed;
>    expect.ok(isSuiteProgressIndicatorRed, "Suite progress indicator is red");
>  
>    //check suite counts
> +  expect.equal(seleniumManager.runCount.getNode().value, "1", "Run count is '1'");

As above: "1 test has run"

@@ +44,5 @@
>    expect.ok(isSuiteProgressIndicatorRed, "Suite progress indicator is red");
>  
>    //check suite counts
> +  expect.equal(seleniumManager.runCount.getNode().value, "1", "Run count is '1'");
> +  expect.equal(seleniumManager.failureCount.getNode().value, "1", "Failure count is '1'");

And "1 test has failed"

::: tests/functional/testFormManager/testDisableFormManager.js
@@ +63,5 @@
>    );
>  
>    controller.assertNodeNotExist(popDownAutoCompList);
> +  expect.equal(firstName.getNode().value, FNAME.substring(0,2),
> +               "First Name is not autocompleted");

Change message to: "First name has not been autocompleted"

@@ +70,5 @@
>    controller.type(lastName, LNAME.substring(0,2));
>    controller.sleep(TIMEOUT);
>    controller.assertNodeNotExist(popDownAutoCompList);
> +  expect.equal(lastName.getNode().value, LNAME.substring(0,2),
> +               "Last Name is not autocompleted");

Change message to: "Last name has not been autocompleted"

::: tests/functional/testFormManager/testSaveFormInformation.js
@@ +55,5 @@
>    }, "Autocomplete popup is open: got '" + popDownAutoCompList.getNode().popupOpen + "', expected 'true'");
>  
>    controller.keypress(firstName, "VK_DOWN", {});
>    controller.click(popDownAutoCompList);
> +  expect.equal(firstName.getNode().value, FNAME, "First Name is autocompleted");

Change message to: "First name has been autocompleted"

@@ +66,5 @@
>    }, "Autocomplete popup is open: got '" + popDownAutoCompList.getNode().popupOpen + "', expected 'true'");
>  
>    controller.keypress(lastName, "VK_DOWN", {});
>    controller.click(popDownAutoCompList);
> +  expect.equal(lastName.getNode().value, LNAME, "Last Name is autocompleted");

Change message to: "Last name has been autocompleted"

::: tests/functional/testPreferences/testPasswordNotSaved.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 the required modules
> +var { assert, expect } = require("../../../lib/assertions");

assert is not being used.

@@ +36,5 @@
>    var userField = new elementslib.ID(controller.tabs.activeTab, "uname");
>    var passField = new elementslib.ID(controller.tabs.activeTab, "Password");
>  
>    controller.waitForElement(userField, TIMEOUT);
> +  expect.equal(userField.getNode().value, "", "Username has been saved");

Message should be: "Username has not been saved"

@@ +37,5 @@
>    var passField = new elementslib.ID(controller.tabs.activeTab, "Password");
>  
>    controller.waitForElement(userField, TIMEOUT);
> +  expect.equal(userField.getNode().value, "", "Username has been saved");
> +  expect.equal(passField.getNode().value, "", "Password has been saved");

Message should be: "Password has not been saved"

::: tests/functional/testPrivateBrowsing/testStartStopPBMode.js
@@ +66,5 @@
>    var longDescElem = new elementslib.ID(controller.tabs.activeTab, "errorLongDescText");
>    var moreInfoElem = new elementslib.ID(controller.tabs.activeTab, "moreInfoLink");
>    controller.waitForElement(longDescElem, TIMEOUT);
> +  expect.equal(longDescElem.getNode().textContent, description,
> +               "Private Browsing description mathces the language files");

Typo: mathces/matches

@@ +68,5 @@
>    controller.waitForElement(longDescElem, TIMEOUT);
> +  expect.equal(longDescElem.getNode().textContent, description,
> +               "Private Browsing description mathces the language files");
> +  expect.equal(moreInfoElem.getNode().textContent, learnMore,
> +               "Text in 'Learn More' link mathces the language files");

Typo: mathces/matches

::: tests/functional/testSecurity/testGreenLarry.js
@@ +38,5 @@
>                                                    "identity-icon-label");
>    var identCountryLabel = new elementslib.ID(controller.window.document,
>                                               "identity-icon-country-label");
> +  expect.equal(identOrganizationLabel.getNode().value, cert.organization,
> +               "Label displays 'Organization'");

Change message to: "Certificate's organization is displayed"

@@ +40,5 @@
>                                               "identity-icon-country-label");
> +  expect.equal(identOrganizationLabel.getNode().value, cert.organization,
> +               "Label displays 'Organization'");
> +  expect.equal(identCountryLabel.getNode().value, '(' + country + ')',
> +               "Label displays '(CountryCode)'");

Change message to: "Certificate's country code is displayed"

@@ +144,4 @@
>    var webIDOwnerLabel = new elementslib.ID(controller.window.document,
>                                             "security-identity-owner-value");
> +  expect.equal(webIDOwnerLabel.getNode().value, cert.organization,
> +               "Owner label matches the Cert Owner");

Change message to: "Owner matches certificate's organization"

@@ +149,4 @@
>    var webIDVerifierLabel = new elementslib.ID(controller.window.document,
>                                                "security-identity-verifier-value");
> +  expect.equal(webIDVerifierLabel.getNode().value, cert.issuerOrganization,
> +               "Verifier label matches the Cert Issuer");

Change message to: "Verifier matches certificate's issuer"

::: tests/functional/testSessionStore/testUndoTabFromContextMenu.js
@@ +55,2 @@
>    var linkId = new elementslib.ID(controller.tabs.activeTab, "id");
> +  expect.equal(linkId.getNode().textContent, "2", "Id on 2nd tab should be 2");

The failure would include the expected value, so the message can just be: "Second tab has correct id"

@@ +71,2 @@
>    linkId = new elementslib.ID(controller.tabs.activeTab, "id");
> +  expect.equal(linkId.getNode().textContent, "1", "Id on 2nd tab should be 1");

As above, change the message to: "Second tab has correct id"

::: tests/functional/testTabbedBrowsing/testOpenInBackground.js
@@ +66,4 @@
>    for each(var tab in TAB_ORDER) {
>      var linkId = new elementslib.ID(controller.tabs.getTab(tab.index), "id");
>      controller.waitForElement(linkId);
> +    assert.equal(parseInt(linkId.getNode().textContent, 10), tab.linkid,

Why do we now need to parse this as an Integer where we didn't before? I would be more inclined to convert tab.linkid a String.

::: tests/functional/testTabbedBrowsing/testOpenInForeground.js
@@ +75,4 @@
>    for each(tab in gTabOrder) {
>      var linkId = new elementslib.ID(controller.tabs.getTab(tab.index), "id");
>      controller.waitForElement(linkId);
> +    assert.equal(parseInt(linkId.getNode().textContent, 10), tab.linkid,

Same as previous comment regarding parsing to an Integer

Comment 3

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0

Changed all messages and used toString() instead of parseInt().

(In reply to Dave Hunt (:davehunt) from comment #2)
> Comment on attachment 681805 [details] [diff] [review]
> Why do we now need to parse this as an Integer where we didn't before? I
> would be more inclined to convert tab.linkid a String.
assertValue[1] and assertText[2] used ==, whereas assert.equal[3] uses ===, so type has to match now.
[1] - https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L945 
[2] - https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L899
[3] - hg.mozilla.org/qa/mozmill-tests/file/default/lib/assertions.js#l239

Comment 4

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

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

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

Looks good. Please provide a link to a passing report in Mozmill Dashboard. Once I can see there are no regressions caused by the patch we can push it to the default branch.

Comment 5

[Alex Lakatos[:AlexLakatos]]

Here they are:
http://mozmill-crowd.blargon7.com/#/functional/report/674977957b923f4905160d1b9a337d9a
http://mozmill-crowd.blargon7.com/#/functional/report/674977957b923f4905160d1b9a33b2f9

Comment 6

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

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

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

Some stuff to fix before we can checkin the patch. See inline comments.

::: lib/tests/testTabView.js
@@ +93,4 @@
>    var tab = tabView.getTabs({filter: "active"})[0];
>    var title = tabView.getTabTitleBox({tab: tab});
> +  assert.equal(title.getNode().textContent, pageTitle,
> +               "Active tab is the Add-ons Website");

Why is that an assert? It has to use expect.

::: tests/functional/testAwesomeBar/testLocationBarSearches.js
@@ +38,3 @@
>    var resultsStringCheck = new elementslib.ID(controller.tabs.activeTab, "term");
> +  assert.equal(resultsStringCheck.getNode().textContent, testString,
> +               "Search term is present in return results count");

While we are touching this line I would love it if we could give the variable a better name. Also the message would benefit from. Reading this I'm not sure what we are testing here.

::: tests/functional/testFormManager/testDisableFormManager.js
@@ +63,5 @@
>    );
>  
>    controller.assertNodeNotExist(popDownAutoCompList);
> +  expect.equal(firstName.getNode().value, FNAME.substring(0,2),
> +               "First name has not been autocompleted");

Why don't we use notEquals here in combination with the whole FNAME string? This looks bogus to me.

@@ +70,5 @@
>    controller.type(lastName, LNAME.substring(0,2));
>    controller.sleep(TIMEOUT);
>    controller.assertNodeNotExist(popDownAutoCompList);
> +  expect.equal(lastName.getNode().value, LNAME.substring(0,2),
> +               "Last name has not been autocompleted");

Same here.

::: tests/functional/testPrivateBrowsing/testAboutPrivateBrowsing.js
@@ +47,5 @@
>    var issueDesc = utils.getEntity(pb.getDtds(), "privatebrowsingpage.issueDesc.normal");
>    var statusText = new elementslib.ID(controller.tabs.activeTab, "errorShortDescTextNormal");
>    controller.waitForElement(statusText);
> +  expect.equal(statusText.getNode().textContent, issueDesc,
> +               "Firefox is not in Private Browsing mode");

This message can lead to a misunderstanding. It should really refer to the status text.

::: tests/functional/testPrivateBrowsing/testStartStopPBMode.js
@@ +68,5 @@
>    controller.waitForElement(longDescElem, TIMEOUT);
> +  expect.equal(longDescElem.getNode().textContent, description,
> +               "Private Browsing description matches the language files");
> +  expect.equal(moreInfoElem.getNode().textContent, learnMore,
> +               "Text in 'Learn More' link matches the language files");

".. displayed correctly". I wouldn't say 'language files'

::: tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
@@ +87,2 @@
>    controller.waitForElement(linkId);
> +  assert.equal(linkId.getNode().textContent, "2", "Link text is correct");

Why an assert and not an expect?

Comment 7

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v3.0

Comment 8

[Alex Lakatos[:AlexLakatos]]

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

addressed Henrik's review

Comment 9

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

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

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

::: tests/functional/testPrivateBrowsing/testAboutPrivateBrowsing.js
@@ +48,5 @@
>    var statusText = new elementslib.ID(controller.tabs.activeTab, "errorShortDescTextNormal");
>    controller.waitForElement(statusText);
> +
> +  var statusTextContent = statusText.getNode().textContent;
> +  expect.equal(statusTextContent, issueDesc, statusTextContent);

The message should refer to the status test content, but should not *be* the status text content. If this failed because the text content was empty then the message would be empty too! A more suitable message would be "Status text indicates we are in private browsing mode".

Comment 10

[Daniela Petrovici]

Attachment: patch v3.1

Changed statusTextContent and added the required error message
Also performed manual changes of some files due to the fact that the repository was updated since patch v3.0 was added

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 697902 [details] [diff] [review]
patch v3.1

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

Looks like all requested changes have been made, tested it on Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/23d8fbdd0190d4b0496d6b129f91d5ba

Comment 12

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

http://hg.mozilla.org/qa/mozmill-tests/rev/0e64c9944774 (default)

Comment 13

[Daniela Petrovici]

Attachment: patch v1.0 [Beta, Release, ESR17]

Patch for Beta which applies clearly on ESR17 and release branches
Aurora does not need a patch due to the fact that it is now FF 20 and the changes from this bug were added to aurora branch
I will add another patch for esr10 branch since this one does not clearly applies

Comment 14

[Daniela Petrovici]

Attachment: patch v1.0 for ESR10

Patch for esr10 branch

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 700271 [details] [diff] [review]
patch v1.0 [Beta, Release, ESR17]

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

This all applies cleanly to me for listed branch.
If you have tested them all and we're not failing, lets have this fixed.

Comment 16

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/a5442bcc6225 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/1d142abb4298 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/b20a5748d0c3 (mozilla-esr17)
http://hg.mozilla.org/qa/mozmill-tests/rev/3c7af2f0c61b (mozilla-esr10)

Comment 17

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

The patches here are causing a regression in at least in two files on the esr10 branch. I haven't tested others so please make sure to do so.

Andreea, we should never set checkin-needed for untested patches!

http://mozmill-ci.blargon7.com/#/functional/report/f25fe2f500e5e4086802832f5214213d

Alex, please fix that ASAP. Thanks.

Comment 18

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

Same for ESR17:

http://mozmill-ci.blargon7.com/#/functional/report/f25fe2f500e5e4086802832f52148d6b

Comment 19

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

And for beta and release given that it is the same patch:

http://mozmill-ci.blargon7.com/#/functional/report/f25fe2f500e5e4086802832f5214d09f

Comment 20

[Alex Lakatos[:AlexLakatos]]

Attachment: follow-up patch v1.0 [esr10]

Patch for esr10 branch
Passing testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/f25fe2f500e5e4086802832f521703f1
http://mozmill-crowd.blargon7.com/#/functional/report/f25fe2f500e5e4086802832f52179730

Comment 21

[Alex Lakatos[:AlexLakatos]]

(In reply to Alex Lakatos[:AlexLakatos] from comment #20)
> Created attachment 702778 [details] [diff] [review]
> follow-up patch v1.0 [esr10]
> 
> Patch for esr10 branch
> Passing testrun:
> http://mozmill-crowd.blargon7.com/#/functional/report/
> f25fe2f500e5e4086802832f521703f1
> http://mozmill-crowd.blargon7.com/#/functional/report/
> f25fe2f500e5e4086802832f52179730
Those were 10.0.11. Here is 10.0.12 just in case:
http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d3006ffa
http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d3002a84

Comment 22

[Alex Lakatos[:AlexLakatos]]

Attachment: follow-up patch v1.0 [esr17, beta, release]

Patch for the beta, release and esr17 branch.
Beta Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/f25fe2f500e5e4086802832f521a2704
http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d3000d5a

Release Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/f25fe2f500e5e4086802832f52192354
http://mozmill-crowd.blargon7.com/#/functional/report/f25fe2f500e5e4086802832f5219a9d1

ESR17 Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/f25fe2f500e5e4086802832f52190ad2
http://mozmill-crowd.blargon7.com/#/functional/report/f25fe2f500e5e4086802832f5218b79e

Comment 23

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

Comment on attachment 702778 [details] [diff] [review]
follow-up patch v1.0 [esr10]

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

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

Comment 24

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

So for esr10 I still see an instance of assertValue:

./tests/functional/testSecurity/testDVCertificate.js:  controller.assertValue(identLabel, gETLDService.getBaseDomainFromHost(cert.commonName));

Comment 25

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

Comment on attachment 702805 [details] [diff] [review]
follow-up patch v1.0 [esr17, beta, release]

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

http://hg.mozilla.org/qa/mozmill-tests/rev/a6904d4e6e65 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/74b1c49115fd (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/11b104dedf99 (esr17)

Comment 26

[Alex Lakatos[:AlexLakatos]]

Attachment: assertValue follow-up patch v1.0 [esr10]

(In reply to Henrik Skupin (:whimboo) from comment #24)
> So for esr10 I still see an instance of assertValue:
> 
> ./tests/functional/testSecurity/testDVCertificate.js: 
> controller.assertValue(identLabel,
> gETLDService.getBaseDomainFromHost(cert.commonName));
That's probably cause the DVCertificate test is different on the esr10 branch. Probably missed while back-porting the initial patch, as that did not touch testDVCertificate.js. Here is a follow-up patch. Running the test locally passes.

Comment 27

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

Comment on attachment 702822 [details] [diff] [review]
assertValue follow-up patch v1.0 [esr10]

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

::: tests/functional/testSecurity/testDVCertificate.js
@@ +30,5 @@
>    // Check the label displays
>    // Format: Organization
>    var identLabel = new elementslib.ID(controller.window.document, "identity-icon-label");
> +  expect.equal(identLabel.getNode().value, gETLDService.getBaseDomainFromHost(cert.commonName),
> +               "Label is displayed");

Remove the comment before and use a more descriptive message here. Label can be everything.

Comment 28

[Daniela Petrovici]

Attachment: assertValue follow-up patch v1.1 [esr10]

Removed the comment and also changed the message

Comment 29

[Daniela Petrovici]

Reports for ESR10 branch are:
http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d30b63af
http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d30b436d

Comment 30

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

Comment on attachment 702895 [details] [diff] [review]
assertValue follow-up patch v1.1 [esr10]

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

::: tests/functional/testSecurity/testDVCertificate.js
@@ +30,3 @@
>    var identLabel = new elementslib.ID(controller.window.document, "identity-icon-label");
> +  expect.equal(identLabel.getNode().value, gETLDService.getBaseDomainFromHost(cert.commonName),
> +               "Identity icon label is displayed");

What's the value of `identLabel.getNode().value`? Isn't it the base domain? In that case the message should read 'Domain name is shown in the identity field'.

Comment 31

[Daniela Petrovici]

Attachment: assertValue follow-up patch v1.2 [esr10]

Changed error message per review. Reports are:
http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d316bb02
http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d316d32f

Comment 32

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

Comment on attachment 703193 [details] [diff] [review]
assertValue follow-up patch v1.2 [esr10]

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

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

::: tests/functional/testSecurity/testDVCertificate.js
@@ +30,3 @@
>    var identLabel = new elementslib.ID(controller.window.document, "identity-icon-label");
> +  expect.equal(identLabel.getNode().value, gETLDService.getBaseDomainFromHost(cert.commonName),
> +               "Domain name is shown in the identity field");

To answer the question I have raised in my last review comments myself:

"message": "Domain name is shown in the identity field - 'mozqa.com' should equal 'mozqa.com'"

So we are fine now.

Comment 33

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

Looks like we are finally done here. Thanks!