Closed Bug 803088 Opened 10 years ago Closed 10 years ago

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

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(firefox18 fixed, firefox19 fixed, firefox20 fixed, firefox-esr10 fixed, firefox-esr17 fixed)

RESOLVED FIXED
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed
firefox-esr10 --- fixed
firefox-esr17 --- fixed

People

(Reporter: whimboo, Assigned: daniela.p98911)

References

Details

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

Attachments

(6 files, 5 obsolete files)

32.56 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
32.52 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
31.89 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
1.89 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
1.12 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
2.27 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
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.
Assignee: nobody → alex.lakatos
Status: NEW → ASSIGNED
Summary: Get rid of controller.assertText() and controller.assertText() in favor of expect.* and assert.* methods → Get rid of controller.assertText() and controller.assertValue() in favor of expect.* and assert.* methods
Attached patch patch v1.0 (obsolete) — Splinter Review
repaced assertValue and assertText, using getNode().value and getNode().textContent
Attachment #681805 - Flags: review?(hskupin)
Attachment #681805 - Flags: review?(dave.hunt)
Comment on attachment 681805 [details] [diff] [review]
patch v1.0

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

::: tests/addons/ide@seleniumhq.org/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
Attachment #681805 - Flags: review?(hskupin)
Attachment #681805 - Flags: review?(dave.hunt)
Attachment #681805 - Flags: review-
Attached patch patch v2.0 (obsolete) — Splinter Review
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
Attachment #681805 - Attachment is obsolete: true
Attachment #682010 - Flags: review?(hskupin)
Attachment #682010 - Flags: review?(dave.hunt)
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.
Attachment #682010 - Flags: review?(hskupin)
Attachment #682010 - Flags: review?(dave.hunt)
Attachment #682010 - Flags: review+
Attachment #682010 - Flags: checkin?
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?
Attachment #682010 - Flags: checkin? → review-
Attached patch patch v3.0 (obsolete) — Splinter Review
Attachment #684481 - Flags: review?(hskupin)
Attachment #682010 - Attachment is obsolete: true
Comment on attachment 684481 [details] [diff] [review]
patch v3.0

addressed Henrik's review
Attachment #684481 - Flags: review?(dave.hunt)
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".
Attachment #684481 - Flags: review?(hskupin)
Attachment #684481 - Flags: review?(dave.hunt)
Attachment #684481 - Flags: review-
Attached patch patch v3.1Splinter Review
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
Attachment #684481 - Attachment is obsolete: true
Attachment #697902 - Flags: review?(hskupin)
Attachment #697902 - Flags: review?(dave.hunt)
Attachment #697902 - Flags: review?(andreea.matei)
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
Attachment #697902 - Flags: review?(hskupin)
Attachment #697902 - Flags: review?(dave.hunt)
Attachment #697902 - Flags: review?(andreea.matei)
Attachment #697902 - Flags: review+
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
Attachment #700271 - Flags: review?(hskupin)
Attachment #700271 - Flags: review?(dave.hunt)
Attachment #700271 - Flags: review?(andreea.matei)
Patch for esr10 branch
Attachment #700272 - Flags: review?(hskupin)
Attachment #700272 - Flags: review?(dave.hunt)
Attachment #700272 - Flags: review?(andreea.matei)
Attachment #700272 - Flags: review?(hskupin)
Attachment #700272 - Flags: review?(dave.hunt)
Attachment #700272 - Flags: review?(andreea.matei)
Attachment #700272 - Flags: review+
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.
Attachment #700271 - Flags: review?(hskupin)
Attachment #700271 - Flags: review?(dave.hunt)
Attachment #700271 - Flags: review?(andreea.matei)
Attachment #700271 - Flags: review+
Attachment #700271 - Attachment description: patch v1.0 for Beta → patch v1.0 [Beta, Release, ESR17]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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 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)
Attachment #702778 - Flags: review?(hskupin)
Attachment #702778 - Flags: review?(dave.hunt)
Attachment #702778 - Flags: review?(andreea.matei)
Attachment #702778 - Flags: review+
Attachment #702778 - Flags: checkin+
So for esr10 I still see an instance of assertValue:

./tests/functional/testSecurity/testDVCertificate.js:  controller.assertValue(identLabel, gETLDService.getBaseDomainFromHost(cert.commonName));
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)
Attachment #702805 - Flags: review?(hskupin)
Attachment #702805 - Flags: review?(dave.hunt)
Attachment #702805 - Flags: review?(andreea.matei)
Attachment #702805 - Flags: review+
Attachment #702805 - Flags: checkin+
(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.
Attachment #702822 - Flags: review?(hskupin)
Attachment #702822 - Flags: review?(dave.hunt)
Attachment #702822 - Flags: review?(andreea.matei)
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.
Attachment #702822 - Flags: review?(hskupin)
Attachment #702822 - Flags: review?(dave.hunt)
Attachment #702822 - Flags: review?(andreea.matei)
Attachment #702822 - Flags: review-
Removed the comment and also changed the message
Attachment #702822 - Attachment is obsolete: true
Attachment #702895 - Flags: review?(hskupin)
Attachment #702895 - Flags: review?(dave.hunt)
Attachment #702895 - Flags: review?(andreea.matei)
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'.
Attachment #702895 - Flags: review?(hskupin)
Attachment #702895 - Flags: review?(dave.hunt)
Attachment #702895 - Flags: review?(andreea.matei)
Attachment #702895 - Flags: review-
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.
Attachment #703193 - Flags: review?(hskupin)
Attachment #703193 - Flags: review?(dave.hunt)
Attachment #703193 - Flags: review?(andreea.matei)
Attachment #703193 - Flags: review+
Attachment #703193 - Flags: checkin+
Looks like we are finally done here. Thanks!
Assignee: alex.lakatos → dpetrovici
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.