Closed Bug 826645 Opened 10 years ago Closed 10 years ago

Add throws() and doesNotThrow() method to the assertions module

Categories

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

defect

Tracking

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

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

People

(Reporter: andrei, Assigned: daniela.p98911)

Details

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

Attachments

(2 files, 2 obsolete files)

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().
Priority: -- → P3
Summary: Add throws() method to the assertions module → Add throws() and doesNotThrow() method to the assertions module
Whiteboard: s=130121 u=enhancement c=assertions p=1
Whiteboard: s=130121 u=enhancement c=assertions p=1
Whiteboard: s=130121 u=enhancement c=assertions p=1
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
Attached patch patch v1.0 (obsolete) — Splinter Review
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?
Attachment #709032 - Flags: feedback?(hskupin)
Attachment #709032 - Flags: feedback?(dave.hunt)
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
Attachment #709032 - Flags: feedback?(hskupin)
Attachment #709032 - Flags: feedback?(dave.hunt)
Attachment #709032 - Flags: feedback-
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.
Attachment #709032 - Flags: feedback-
Attached patch patch v1.1 (obsolete) — Splinter Review
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
Attachment #709032 - Attachment is obsolete: true
Attachment #709674 - Flags: review?(hskupin)
Attachment #709674 - Flags: review?(dave.hunt)
Attachment #709674 - Flags: review?(andreea.matei)
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.
Attachment #709674 - Flags: review?(hskupin)
Attachment #709674 - Flags: review?(dave.hunt)
Attachment #709674 - Flags: review?(andreea.matei)
Attachment #709674 - Flags: review-
Attached patch patch v1.2Splinter Review
removed the errors library
Attachment #709674 - Attachment is obsolete: true
Attachment #710160 - Flags: review?(andreea.matei)
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?
Attachment #710160 - Flags: review?(andreea.matei) → review+
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)
Attachment #710160 - Flags: checkin+
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)
Status: ASSIGNED → RESOLVED
Closed: 10 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.