809434 - Get rid of controller.assertChecked() in favor of expect.* and assert.* methods

Status: RESOLVED FIXED

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

Description

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

Similar to bug 782918 we should update our tests and replace any instance of controller.assertChecked() with the appropriate assert.* or expect.* call.

Comment 1

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

Do we have an update on that bug? Alex, if you can't work on it please ensure it find the appropriate owner.

Comment 2

[mario garbi]

Attachment: patch v1.0

changed controller.assertChecked with assert.*

Comment 3

[Andreea Matei [:AndreeaMatei]]

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

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

::: tests/functional/testPreferences/testDefaultPhishingEnabled.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Include necessary modules
>  var prefs = require("../../../lib/prefs");
> +var { assert } = require("../../../lib/assertions");

This needs to be above prefs.

@@ +44,5 @@
>  
>  /**
>   * Map test functions to litmus tests
>   */
>  // testDefaultPhishingEnabled.meta = {litmusids : [8290]};

I would like to have this removed since we're here and no longer use litmus.

::: tests/functional/testPreferences/testDefaultSecurityPrefs.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Include necessary modules
>  var prefs = require("../../../lib/prefs");
> +var { assert } = require("../../../lib/assertions");

This needs to be above prefs.

@@ +44,5 @@
>  
>  /**
>   * Map test functions to litmus tests
>   */
>  // testDefaultSecurityPreferences.meta = {litmusids : [8060]};

I would like to have this removed since we're here and no longer use litmus.

Comment 4

[mario garbi]

Attachment: patch v1.1

Modified patch based on review.

Comment 5

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

Comment on attachment 693811 [details] [diff] [review]
patch v1.1

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

Looks good. Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/7be14decebeb

Comment 6

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

Let's get this backported once we see there are no regressions. Please test and provide backport patches as needed.

Comment 7

[mario garbi]

Attachment: patch v1.2 Non-Default

Works with all branches other than Default (esr, aurora, beta, release).

esr10 : http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db86db3c

esr17 : http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db8743a1

release 17 : http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db88b354

beta 18 : http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db8901d8

aurora 19 : http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db8a3fb2

Comment 8

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

Comment on attachment 693901 [details] [diff] [review]
patch v1.2 Non-Default

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

::: tests/functional/testPreferences/testDefaultPhishingEnabled.js
@@ +36,5 @@
>  
>    // Verify Block Attack Sites and Reported Web Forgeries are checked by default
>    controller.waitForElement(attackElem, gTimeout);
> +  assert.ok(attackElem.getNode().checked, "Block Attack Sites checkbox is checked");
> +  assert.ok(forgeryElem.getNode().checked, "Reported Web Forgeries checkbox is checked");

Why are those two lines an assert while it has to be an expect? Please come up with a follow-up patch for default. Afterward you can come up with a new backport patch.

::: tests/functional/testPreferences/testDefaultSecurityPrefs.js
@@ +36,5 @@
>    var sslPref = new elementslib.ID(controller.window.document, "useSSL3");
>    var tlsPref = new elementslib.ID(controller.window.document, "useTLS1");
>    controller.waitForElement(sslPref, gTimeout);
> +  assert.ok(sslPref.getNode().checked, "SSL3 Preferences checkbox is checked");
> +  assert.ok(tlsPref.getNode().checked, "TLS1 Preferences checkbox is checked");

Same here.

Comment 9

[mario garbi]

Attachment: patch v1.3 Default

Changed "assert.ok" into "expect.ok" as requested above.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 694295 [details] [diff] [review]
patch v1.3 Default

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

Just few more things and we're good to go.

::: tests/functional/testFormManager/testClearFormHistory.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 { assert, expect } = require("../../../lib/assertions");

No need for expect here, it is not used.

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

If you changed to expect only, there is no need to declare assert too, it is not used.

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

Same here, no need for assert.

Comment 11

[mario garbi]

Attachment: patch v1.4 Default

Removed { assert } where it was not used used any more.

Comment 12

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 694351 [details] [diff] [review]
patch v1.4 Default

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

This patch does not apply cleanly because you have already checked in the previous patch (with asserts instead of expect).
You need to make a follow-up patch with the changes Henrik and I requested (replacing those asserts with expect() and make sure what modules you import).
Please always work with clean repository and don't mark the patches that are already checked-in as obsolete.

Thanks.

Comment 13

[mario garbi]

Attachment: patch v1.5 FollowUp for Default

Follow up patch that changes Assert into Expect as requested.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 695964 [details] [diff] [review]
patch v1.5 FollowUp for Default

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

Looks good now. Thanks.
We'll need this changes backported as well.

Comment 15

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

Comment on attachment 695964 [details] [diff] [review]
patch v1.5 FollowUp for Default

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

This patch doesn't contain any commit message. Please make sure in the future there is one set before uploading a patch.

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

Comment 16

[mario garbi]

Attachment: patch v1.6 Non-Default

Patch ported for all other branches as requested. Reports coming in a second.

Comment 17

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

No need to land this on aurora given that the merge moved it forward from default already.

Other branches:
http://hg.mozilla.org/qa/mozmill-tests/rev/125d828535a5 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/2a433439be6d (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/1c7990d21541 (esr17)
http://hg.mozilla.org/qa/mozmill-tests/rev/687e98c1b731 (esr10)