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

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

changed controller.assertChecked with assert.*

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.

Modified patch based on review.

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

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

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 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.

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

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.

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

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.

Follow up patch that changes Assert into Expect as requested.

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 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)

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

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)