Port the throws() method from Mozmill Extension into the tests assertion module ( mozmill-tests/lib/assertions.js ) Relevant code: https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/assertions.js#L416

We also need doesNotThrow().

I am not sure this is what needs to be done here. I have added the methods from the link in comment #1 and added them to the assertions.js file. Can you please tell me if this needs to be changed?

Comment on attachment 709032 [details] [diff] [review] patch v1.0 Review of attachment 709032 [details] [diff] [review]: ----------------------------------------------------------------- What about assert.throws and assert.doesNotThrow? ::: lib/assertions.js @@ +436,5 @@ > +/* Tests whether a code block throws the expected exception > + * class. helper for throws() and doesNotThrow() > + * adapted from node.js's assert._throws() > + * https://github.com/joyent/node/blob/master/lib/assert.js > + * nit: trailing whitespace @@ +466,5 @@ > + if (!shouldThrow && this._expectedException(actual, aExpected)) { > + return this._test(false, aMessage, 'Got unwanted exception'); > + } > + > + if ((shouldThrow && actual && aExpected && nit: trailing space @@ +467,5 @@ > + return this._test(false, aMessage, 'Got unwanted exception'); > + } > + > + if ((shouldThrow && actual && aExpected && > + !this._expectedException(actual, aExpected)) || (!shouldThrow && actual)) { If !shouldThrow why would we expect actual and throw actual? @@ +474,5 @@ > + return this._test(true, aMessage); > +}; > + > +/* Tests whether an actual and an expected error are the same > + * nit: trailing space

I will modify the patch for the rest of the items above (In reply to Dave Hunt (:davehunt) from comment #3) > Comment on attachment 709032 [details] [diff] [review] > patch v1.0 > > Review of attachment 709032 [details] [diff] [review]: > ----------------------------------------------------------------- > If !shouldThrow why would we expect actual and throw actual? If we are not expecting to throw an error (!shouldThrow), but we tried the code and we got the error, then we just throw it. Would it be ok if I just use this._test here as well?

(In reply to Daniela Petrovici from comment #4) > I will modify the patch for the rest of the items above > > (In reply to Dave Hunt (:davehunt) from comment #3) > > Comment on attachment 709032 [details] [diff] [review] > > patch v1.0 > > > > Review of attachment 709032 [details] [diff] [review]: > > ----------------------------------------------------------------- > > If !shouldThrow why would we expect actual and throw actual? > > If we are not expecting to throw an error (!shouldThrow), but we tried the > code and we got the error, then we just throw it. > > Would it be ok if I just use this._test here as well? I see, I misread the code. Please put a new line before the return this._test(true, aMessage); to separate it from the block beforehand. Thanks. :)

Comment on attachment 709032 [details] [diff] [review] patch v1.0 Review of attachment 709032 [details] [diff] [review]: ----------------------------------------------------------------- This is not a straight port from Mozmill. It misses code at least for building the message if aExpected is of type Error. not sure what else is missing. We have to use exactly the same, otherwise we will run into problems later.

patch v1.1 - changed the code to exactly match the one given in comment #1 Changed the test_assertions.js function so that it resembles the one here: https://github.com/whimboo/mozmill-test-refactor/blob/69388a84e7c927b831d4470972cb04e1ab59110b/lib/tests/test_assertions.js

Comment on attachment 709674 [details] [diff] [review] patch v1.1 Review of attachment 709674 [details] [diff] [review]: ----------------------------------------------------------------- It's the same code as in Mozmill now, but there's a new lib imported that has to be removed. ::: lib/tests/test_assertions.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/. */ > > var {assert, expect, Expect} = require("../assertions"); > +var errors = require("../errors"); We don't have this library.

removed the errors library

Comment on attachment 710160 [details] [diff] [review] patch v1.2 Review of attachment 710160 [details] [diff] [review]: ----------------------------------------------------------------- It's identical now and has the assertions test. Henrik/Dave, I assume we want this backported?

Comment on attachment 710160 [details] [diff] [review] patch v1.2 Review of attachment 710160 [details] [diff] [review]: ----------------------------------------------------------------- http://hg.mozilla.org/qa/mozmill-tests/rev/70bb089d2395 (default)

The patch for default clearly applies on all branches (tested: Aurora, Beta, Release and ESR17). Since throws is only used in test_assertions I have run the test on all branches after importing the patch for default.

Transplanted as: http://hg.mozilla.org/qa/mozmill-tests/rev/2df835129fb3 (mozilla-aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/94beee724e4b (mozilla-beta) http://hg.mozilla.org/qa/mozmill-tests/rev/ba62e4fe90a7 (mozilla-release) http://hg.mozilla.org/qa/mozmill-tests/rev/9db3ae7e0b95 (mozilla-esr17)