637880 - Pull in assert/expect shared module and expose to global namespace in tests

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Heather Arthur [:harth]]

We should pull in the shared modules created by bug 634225 and expose 'assert' and 'expect' to the global namespace of tests.

Comment 1

[Heather Arthur [:harth]]

Attachment: add assertions.js and assert and expect to global test namespace

The assert and expect modules in the global namespace. Takes the assertions.js and error.js (we could consider taking AssertionError from errors.js and throwing it in assertions.js) from the mozmill-api-refactor repo https://github.com/geoelectric/mozmill-api-refactor/. The only mod is that it gets rid of the inheritance module and uses straight prototype inheritance.

Comment 2

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

Why do we have to carry assertions.js through securable modules? We don't do this for any other js module under resource. Also the patch is missing the assertions.js and error.js files.

Comment 3

[Heather Arthur [:harth]]

(In reply to comment #2)
> Why do we have to carry assertions.js through securable modules? We don't do
> this for any other js module under resource. 
Because it's a CommonJS module. We could change it to a jsm, but while we've got a CommonJS loader we might as well use the nicer syntax. Is my thinking.

> Also the patch is missing the assertions.js and error.js files.
My bad, thanks for the catch!

Comment 4

[Heather Arthur [:harth]]

Attachment: patch + assertions.js and errors.js

added missing files

Comment 5

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

Heather, I think instead of using our errors.js module you should use the inheritance method Andrew has implemented for the TimeoutError in Mozmill's utils.js file. I'm even planning to port his change back into our errors module.

Comment 6

[Heather Arthur [:harth]]

(In reply to comment #5)
> Heather, I think instead of using our errors.js module you should use the
> inheritance method Andrew has implemented for the TimeoutError in Mozmill's
> utils.js file. I'm even planning to port his change back into our errors
> module.
okay, I might just pull in AssertionError. Will you guys need your own AssertionError if assert is pulled into Mozmill itself?

Comment 7

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

No, once it is available in Mozmill we don't need our own error class.

Oh, and please also pull in the tests for the assertions module which are under modules/tests/test_assertions.js.

Comment 8

[Heather Arthur [:harth]]

> Oh, and please also pull in the tests for the assertions module which are under
> modules/tests/test_assertions.js.

we don't expose Expect() to the global namespace, so that test_assertions.js  doesn't work as is. node tests its assert module using its own assert.throws() function:

https://github.com/joyent/node/blob/master/test/simple/test-assert.js

Inclined to go that route.

Comment 9

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

Ah, I kinda like those two methods and wonder if we should implement those:

assert.throws();
assert.doesNotThrow();

Comment 10

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

If you decide to implement them, I'd suggest:

expect/assert.throws(callback, [classname], message);
expect/assert.doesNotThrow(callback, [classname], message);

...where classname (e.g. "UnexpectedError") lets you check for a specific class of exception. All others should bubble out, as they're unexpected. If not supplied, checks for any exception and nothing should bubble out.

Comment 11

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

Yes, that is what I wanted to do. A check for the exception class would be kinda helpful. I have filed bug 642081.

Heather, I will try to get this fixed this week.

Comment 12

[cmtalbert]

Comment on attachment 519202 [details] [diff] [review]
patch + assertions.js and errors.js

>diff --git a/mozmill/mozmill/extension/resource/modules/assertions.js b/mozmill/mozmill/extension/resource/modules/assertions.js
>new file mode 100644
>index 0000000..01e4940
>--- /dev/null
>+Expect.prototype = {
>+
>+  /**
>+   * Find the frame to use for logging the test result. If a start frame has
>+   * been specified, we walk down the stack until a frame with the same filename
>+   * as the start frame has been found. The next file in the stack will be the
>+   * frame to use for logging the result.
>+   *
>+   * @returns {object} Frame of the stack to use for logging the result.
>+   */
>+  _findCallerFrame: function Expect__findCallerFrame() {
This function does seem like the right function to have here.  However, I have to ask if there are any other instances where we are going to need this stack walker code for reporting?  Also, we don't have the "debug" flag wired in here, so we cannot have a way to see harness level errors while in debug mode.  I really think this code should be in mozmillFrame and simply be called from mozmillFrame by this code. That way we can ensure this stack walk code is called if the test throws an exception that is not part of an except/assertion message.
>+
>+  /**
>+   * Test the condition and mark test as passed or failed
>+   *
>+   * @param {boolean} aCondition
>+   *   Condition to test.
>+   * @param {string} aMessage
>+   *   Message to show for the test result
>+   * @param {string} aDiagnosis
>+   *   Diagnose message to show for the test result
>+   * @returns {boolean} Result of the test.
>+   */
>+  _test: function Expect__test(aCondition, aMessage, aDiagnosis) {
>+    let diagnosis = aDiagnosis || "";
>+    let message = aMessage || "";
>+
>+    if (aDiagnosis)
>+      message = aMessage ? message + " - " + aDiagnosis : aDiagnosis;
This is good, but what is 'diagnosis' for?  It's an unused varaible at this point. I think maybe you meant to say if(diagnosis)?

>+  /**
>+   * Test if both specified values are identical.
>+   *
>+   * @param {boolean|string|number|object} aValue
>+   *   Value to test.
>+   * @param {boolean|string|number|object} aExpected
>+   *   Value to strictly compare with.
>+   * @param {string} aMessage
>+   *   Message to show for the test result
>+   * @returns {boolean} Result of the test.
>+   */
>+  equal: function Expect_equal(aValue, aExpected, aMessage) {
>+    let condition = (aValue === aExpected);
>+    let diagnosis = "got '" + aValue + "', expected '" + aExpected + "'";
>+  
>+    return this._test(condition, aMessage, diagnosis);
>+  },
I like the way these generate diagnosis.  That's very clever.
>+
>+
>+var Assert = function(stackFrame) {
>+		Expect.call(this, stackFrame);
>+}
>+
Again, I think this should live on mozmillFrame.

>+Assert.prototype = new Expect();
>+
>+/**
>+ * The Assert class implements fatal assertions, and can be used in cases
>+ * when a failing test has to directly abort the current test function. All
>+ * remaining tasks will not be performed.
>+ *
>+ * @class Base class for fatal assertions
>+ * @constructs
>+ * @extends assertions.Expect
>+ * @requires errors.AssertionError
>+ * @param {object} [aStartFrame=Components.stack]
>+ *   Frame of the stack to start from for the logging. Per default
>+ *   Components.stack should be used when creating an instance of that class.
>+ */
>+Assert.prototype.initialize = function Assert(aStartFrame) {
>+  this.parent(aStartFrame);
>+}
>+
>+// Export of variables
>+assertions.expect = new Expect(Components.stack);
>+assertions.assert = new Assert(Components.stack);
>+
I'd expect these constructors to call Components.stack and get the start_frame and then have that information for forwarding down to mozmillFrame to get the calling stack.  But I don't think that they need anything in their constructors at all by default

>+++ b/mozmill/mozmill/extension/resource/modules/errors.js
>+ * @name errors
>+ * @namespace Defines error classes to be used for exceptions.
>+ */
>+var errors = exports;
>+
>+
So, one thing I don't understand about your error class.  Over in assert.js you instantiate the AssertionError with the aResult object which is a nice rich object.  But the constructor of the base Error class and the constructError classes all take strings.  How does that work?

>+/**
>+ * Generator for custom error classes
>+ *
>+ * @memberOf errors
>+ * @param {String} aClassName Class name of the custom error
>+ */
>+function constructError(aClassName) {
>+  var constructor = function(aMessage, aFileName, aLineNumber, aFunction) {
>+    this.name = aClassName;
>+    this.message = aMessage;
>+    this.fileName = aFileName;
>+    this.lineNumber = aLineNumber;
>+    this.function = aFunction;
>+
>+    return this;

>+  constructor.name = aClassName || Error.prototype.name;
>+  constructor.prototype.toString = Error.prototype.toString;


>diff --git a/mozmill/mozmill/extension/resource/modules/frame.js b/mozmill/mozmill/extension/resource/modules/frame.js
>index 65b33fd..4205fbb 100644
>--- a/mozmill/mozmill/extension/resource/modules/frame.js
>+++ b/mozmill/mozmill/extension/resource/modules/frame.js
>@@ -52,15 +52,23 @@ var aConsoleService = Components.classes["@mozilla.org/consoleservice;1"].
>      getService(Components.interfaces.nsIConsoleService);
> var ios = Components.classes["@mozilla.org/network/io-service;1"]
>                     .getService(Components.interfaces.nsIIOService);
>-var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>+var subscriptLoader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>                     .getService(Components.interfaces.mozIJSSubScriptLoader);
> var uuidgen = Components.classes["@mozilla.org/uuid-generator;1"]
>-                    .getService(Components.interfaces.nsIUUIDGenerator);  
>+                    .getService(Components.interfaces.nsIUUIDGenerator);
> 
> var backstage = this;
> 
> var persisted = {};
> 
>+var moduleLoader = new securableModule.Loader({
>+  rootPaths: ["resource://mozmill/modules/"],
>+  defaultPrincipal: "system",
>+  globals : { Cc: Components.classes,
>+              Ci: Components.interfaces,
>+              Cu: Components.utils}
Why did you not inclue Cr: Components.results?  

> arrayRemove = function(array, from, to) {
>   var rest = array.slice((to || from) + 1 || array.length);
>   array.length = from < 0 ? array.length + from : from;
>@@ -86,16 +94,24 @@ var loadFile = function(path, collector) {
>   file.initWithPath(path);
>   var uri = ios.newFileURI(file).spec;
> 
>-  var module = {};  
>-  module.collector = collector
>   loadTestResources();
>-  module.mozmill = mozmill;
>-  module.elementslib = elementslib;
>-  module.persisted = persisted;
>-  module.Cc = Components.classes;
>-  module.Ci = Components.interfaces;
>-  module.Cu = Components.utils;
>-  module.log = log;
>+  try {
>+  var assertions = moduleLoader.require("./assertions");
>+  } catch(e) { dump(e + "\n"); 
>+    }
>+  dump(JSON.stringify(assertions) + "\n");
>+  var module = {  
>+    collector:  collector,
>+    mozmill: mozmill,
>+    elementslib: elementslib,
>+    persisted: persisted,
>+    Cc: Components.classes,
>+    Ci: Components.interfaces,
>+    Cu: Components.utils,
include Cr
>+    log: log,
>+    assert: assertions.assert,
>+    expect: assertions.expect
>+  }
> 
>   module.require = function (mod) {
>     var loader = new securableModule.Loader({
>@@ -117,7 +133,7 @@ var loadFile = function(path, collector) {
>     collector.current_path = path;
>   }
>   try {
>-    loader.loadSubScript(uri, module);
>+    subscriptLoader.loadSubScript(uri, module);
>   } catch(e) {
>     events.fail(e);
>     var obj = {
>diff --git a/mozmill/test/test_assertions.js b/mozmill/test/test_assertions.js
>new file mode 100644
>index 0000000..0a31feb
>--- /dev/null
>+++ b/mozmill/test/test_assertions.js
>@@ -0,0 +1,7 @@
>+var setupModule = function(module) {
>+}
>+
>+var testAssert = function() {
>+  assert.ok(true, "wooo");
>+  expect.ok(true, "wwoo");
>+}
We really need a better test here.  We should include a test that will be expected to fail as well. To ensure we can actually instantiate the AssertionError properly (cause it looks like it will fail to me, unless something interesting is going on)

Good approach, the patch looks great, but I really want to see that framestackwalker stuff integrated in frame.js properly with the debug control setting like we discussed and some more complete testing. Thanks Henrik and Heather.
>\ No newline at end of file

Comment 13

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

(In reply to comment #12)
> >+  _findCallerFrame: function Expect__findCallerFrame() {
> This function does seem like the right function to have here.  However, I have
> to ask if there are any other instances where we are going to need this stack
> walker code for reporting?  Also, we don't have the "debug" flag wired in here,
> so we cannot have a way to see harness level errors while in debug mode.  I
> really think this code should be in mozmillFrame and simply be called from
> mozmillFrame by this code. That way we can ensure this stack walk code is
> called if the test throws an exception that is not part of an except/assertion
> message.

Exactly. I have written that as a temporary solution until Mozmill frame is able to report the correct frame from within the test for those things. Feel free whatever you want to do with it. Move it around, remove it, or whatever else makes sense to get it implemented in core. If we can't do it now lets file a follow-up bug?

> >+  _test: function Expect__test(aCondition, aMessage, aDiagnosis) {
> >+    let diagnosis = aDiagnosis || "";
> >+    let message = aMessage || "";
> >+
> >+    if (aDiagnosis)
> >+      message = aMessage ? message + " - " + aDiagnosis : aDiagnosis;
> This is good, but what is 'diagnosis' for?  It's an unused varaible at this
> point. I think maybe you meant to say if(diagnosis)?

Yes, that makes no sense. I will come up with a follow-up for our module.

> >+  equal: function Expect_equal(aValue, aExpected, aMessage) {
> >+    let condition = (aValue === aExpected);
> >+    let diagnosis = "got '" + aValue + "', expected '" + aExpected + "'";
> >+  
> >+    return this._test(condition, aMessage, diagnosis);
> >+  },
> I like the way these generate diagnosis.  That's very clever.

Basically I have used it from Mochitests but extended the schema so each assertion method is using its own content. The output is very verbose and already helps us a lot in the API refactoring project.

> >+assertions.expect = new Expect(Components.stack);
> >+assertions.assert = new Assert(Components.stack);
> >+
> I'd expect these constructors to call Components.stack and get the start_frame
> and then have that information for forwarding down to mozmillFrame to get the
> calling stack.  But I don't think that they need anything in their constructors
> at all by default

Looks like that parameter is really not necessary for the constructors. I have removed them now locally and all the assertions output look the same. 

> So, one thing I don't understand about your error class.  Over in assert.js you
> instantiate the AssertionError with the aResult object which is a nice rich
> object.  But the constructor of the base Error class and the constructError
> classes all take strings.  How does that work?

No, AssertionError gets strings as the base error class. We do not pass in an object anymore. See the generator class:

> >+function constructError(aClassName) {
> >+  var constructor = function(aMessage, aFileName, aLineNumber, aFunction) {
> >+    this.name = aClassName;
> >+    this.message = aMessage;
> >+    this.fileName = aFileName;
> >+    this.lineNumber = aLineNumber;
> >+    this.function = aFunction;

> We really need a better test here.  We should include a test that will be
> expected to fail as well. To ensure we can actually instantiate the
> AssertionError properly (cause it looks like it will fail to me, unless
> something interesting is going on)

As talked with Heather I have written a rich set of tests for all methods, also for the negative case. We should really use those. See:

https://github.com/geoelectric/mozmill-api-refactor/blob/master/modules/tests/test_assertions.js

Also with your points re startFrame that change simplifies a lot!

Thanks for your feedback Clint! I have uploaded a patch with those changes to bug 642409. Next time we should try to get sorted out those issues earlier so we don't port stuff back and forth.

Comment 14

[Heather Arthur [:harth]]

Attachment: patch with throws() and Henrik

Comment 15

[Heather Arthur [:harth]]

Attachment: patch with throws() and Henrik's test

This patch adds the throws() and doesNotThrow() from bug 642081 and Henrik's much more comprehensive test_assertions.js that uses the assert.throw() to test each assertion method.

Comment 16

[Heather Arthur [:harth]]

Attachment: patch with throws() and Henrik's test

Comment 17

[Heather Arthur [:harth]]

Comment on attachment 520001 [details] [diff] [review]
patch with throws() and Henrik's test

patch with throws/doesNotThrow(), Henrik's test, and changes from bug 642409.

Comment 18

[cmtalbert]

(In reply to comment #17)
> Comment on attachment 520001 [details] [diff] [review]
> patch with throws() and Henrik's test
> 
> patch with throws/doesNotThrow(), Henrik's test, and changes from bug 642409.

What happened to the frame munching stuff?  I don't see that in here at all now.  Are we tracking that by another bug somewhere now or did that get lost in the shuffle?

Comment 19

[Heather Arthur [:harth]]

> What happened to the frame munching stuff?  I don't see that in here at all
> now.  Are we tracking that by another bug somewhere now or did that get lost in
> the shuffle?

I thought bug 607450 took care of that?

Comment 20

[cmtalbert]

Comment on attachment 520001 [details] [diff] [review]
patch with throws() and Henrik's test

>+++ b/mozmill/mozmill/extension/resource/modules/assertions.js
>+  /* adapted from node.js's assert.throws()
>+     https://github.com/joyent/node/blob/master/lib/assert.js
>+  */
>+  _throws : function(shouldThrow, block, expected, message) {
That link doesn't provide enough documentation on how this function works.  In particular, can you call out what 'expected' is allowed to be?
A comment detailing that would be great.

>+  throws : function(block, /*optional*/error, /*optional*/message) {
>+    return this._throws.apply(this, [true].concat(Array.prototype.slice.call(arguments)));
>+  },
>+
>+  doesNotThrow : function(block, /*optional*/error, /*optional*/message) {
>+    return this._throws.apply(this, [false].concat(Array.prototype.slice.call(arguments)));
>+  },
>+}
>+
Both these look like they could use a little more documentation as well since they are the public interfaces here.

>+function expectedException(actual, expected) {
This seems like a private function, shouldn't it be _expectedException?

Otherwise this looks good, so r+ with those changes.  Sorry about forgetting that we put the stack walking stuff in python.  My bad.

Comment 21

[Heather Arthur [:harth]]

Ok, it's in:

https://github.com/mozautomation/mozmill/commit/1e19f1f5b64f564a80b5fd534bb5c209f4c7cca1

Comment 22

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

Comment on attachment 520001 [details] [diff] [review]
patch with throws() and Henrik's test

>+  _throws : function(shouldThrow, block, expected, message) {

Just a side-note. With those imported methods we are starting again to be inconsistent in the naming schema of parameters. If you don't mind for code in Mozmill just skip this comment.

Comment 23

[Heather Arthur [:harth]]

(In reply to comment #22)
> Comment on attachment 520001 [details] [diff] [review]
> patch with throws() and Henrik's test
> 
> >+  _throws : function(shouldThrow, block, expected, message) {
> 
> Just a side-note. With those imported methods we are starting again to be
> inconsistent in the naming schema of parameters. If you don't mind for code in
> Mozmill just skip this comment.

Yeah, in your automation code you use the aParam notation. Core mozmill uses regular names, and this code was ported directly from node.js.

Comment 24

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

(In reply to comment #23)
> > Just a side-note. With those imported methods we are starting again to be
> > inconsistent in the naming schema of parameters. If you don't mind for code in
> > Mozmill just skip this comment.
> 
> Yeah, in your automation code you use the aParam notation. Core mozmill uses
> regular names, and this code was ported directly from node.js.

But exactly this makes code inside a module inconsistent. IMO your team should make a decision which way you want to support, but please don't mix it where-ever code from different sources gets pulled in.

Comment 25

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

I have to reopen because the current implementation is not finished. Here the reasons:

* The AssertionError class you define in assertions.js is not exported. So no test can make use of it. That means each throws call cannot be used for expected AssertionError failures. There should also be tests for the AssertionError class and I assume those will fail for the same reason why we weren't able to use classes but had to fallback to Error.name.

* assertion.js should export the Expect and Assert class and not instances of both classes.

* The test module for assertions has two test functions. As what we probably wanna do is only having a single test function in a module. That means the testAssert function doesn't get executed.

* Nit: As mentioned earlier the documentation is missing. Also the newly added methods do not have have a name and are not put into the right place.

Not sure if there will be more, but those items will not make it fully usable at the moment because the throw part is broken.

Comment 26

[Heather Arthur [:harth]]

(In reply to comment #25)
> I have to reopen because the current implementation is not finished. Here the
> reasons:
> 
> * The AssertionError class you define in assertions.js is not exported. So no
> test can make use of it. That means each throws call cannot be used for
> expected AssertionError failures. There should also be tests for the
> AssertionError class and I assume those will fail for the same reason why we
> weren't able to use classes but had to fallback to Error.name.

It's exported on assert.AssertionError, and test_assertions.js uses this.

> 
> * assertion.js should export the Expect and Assert class and not instances of
> both classes.

I don't see how it makes a difference, there's no state stored in either class.


> * The test module for assertions has two test functions. As what we probably
> wanna do is only having a single test function in a module. That means the
> testAssert function doesn't get executed.

Are you saying it doesn't get executed right now? If so, for me both test functions do. If that doesn't work then I think its a mozmill bug?

> 
> * Nit: As mentioned earlier the documentation is missing. Also the newly added
> methods do not have have a name and are not put into the right place.

Not sure what you mean by the methods not having a name? We added the docstring comments when checked in https://github.com/mozautomation/mozmill/commit/1e19f1f5b64f564a80b5fd534bb5c209f4c7cca1. Where does the documentation live?

Comment 27

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

(In reply to comment #26)
> It's exported on assert.AssertionError, and test_assertions.js uses this.

Sorry, that one I have missed. It's kinda hidden.

> > * assertion.js should export the Expect and Assert class and not instances of
> > both classes.
> 
> I don't see how it makes a difference, there's no state stored in either class.

There will be no way to work with those classes in a good way, i.e. creating sub classes.

> > * The test module for assertions has two test functions. As what we probably
> > wanna do is only having a single test function in a module. That means the
> > testAssert function doesn't get executed.
> 
> Are you saying it doesn't get executed right now? If so, for me both test
> functions do. If that doesn't work then I think its a mozmill bug?

This is a change which will be done in the near future. I don't remember the bug number right now.

> Not sure what you mean by the methods not having a name? We added the 

You have set names on the prototype but the functions have no name. That's what would be helpful for debugging purposes:

doesNotThrow: function Expect_doesNotThrow(...)


> docstring comments when checked in
> https://github.com/mozautomation/mozmill/commit/1e19f1f5b64f564a80b5fd534bb5c209f4c7cca1.
> Where does the documentation live?

Not consistently. AssertionError doesn't have any of the params documented, also optional params are enclosed in [] brackets.

Also I do not understand why we need the assert.throws around an expect call:

      assert.throws(function() {
        expect[test.fun].apply(expect, test.params);
      }, test.result.throws, message);

Expect should never throw an error if the assertion condition hasn't been met. The following code has to add a failure only and should not re-throw the Error. 

expect.throws(function () { throw new Error(); }, Error);

So why we would have to enclose it into assert.throws?

Comment 28

[Heather Arthur [:harth]]

(In reply to comment #27)
> > > * assertion.js should export the Expect and Assert class and not instances of
> > > both classes.
> > 
> > I don't see how it makes a difference, there's no state stored in either class.
> 
> There will be no way to work with those classes in a good way, i.e. creating
> sub classes.

We can export both the class and instances if you want, but I'm hesitant about why you'd want to subclass it. Anything you think you'd want on the assert module we should try to get in the core code, and we probably don't want a bunch of superfluous assert methods in a shared module.


> You have set names on the prototype but the functions have no name. That's what
> would be helpful for debugging purposes:
> 
> doesNotThrow: function Expect_doesNotThrow(...)

Cool, I'll change this


> Also I do not understand why we need the assert.throws around an expect call:
> 
>       assert.throws(function() {
>         expect[test.fun].apply(expect, test.params);
>       }, test.result.throws, message);
> 
> Expect should never throw an error if the assertion condition hasn't been met.
> The following code has to add a failure only and should not re-throw the Error. 
> 
> expect.throws(function () { throw new Error(); }, Error);
> 
> So why we would have to enclose it into assert.throws?

I'm fine with either way, Geo, do you have any opinions on this?

Comment 29

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

(In reply to comment #28)
> We can export both the class and instances if you want, but I'm hesitant about
> why you'd want to subclass it. Anything you think you'd want on the assert
> module we should try to get in the core code, and we probably don't want a
> bunch of superfluous assert methods in a shared module.

Personally I do not like the way how you i.e. degrade the Expect class in the test_assertions.js module by resetting the _logFail method:

expect._logFail = function() {};

That's why we have created the SoftExpect class in our version of the test. The same will apply to additional methods we want to have in our modules. Please do not stop us from doing so. Mozmill is too inflexible in terms of adding new methods and pushing a new release immediately. From our experience in the last two years it can take ages until features will find their way into Mozmill. Further I also would only offer proven features for inclusion.

So what is the problem with the following code in frame.js:

+    assert: new assertions.Assert(),
+    expect: new assertions.Expect() 

> > Also I do not understand why we need the assert.throws around an expect call:
> > 
> >       assert.throws(function() {
> >         expect[test.fun].apply(expect, test.params);
> >       }, test.result.throws, message);
> > 
> > Expect should never throw an error if the assertion condition hasn't been met.
> > The following code has to add a failure only and should not re-throw the Error. 
> > 
> > expect.throws(function () { throw new Error(); }, Error);
> > 
> > So why we would have to enclose it into assert.throws?
> 
> I'm fine with either way, Geo, do you have any opinions on this?

The Expect class exists because we want to have non-fatal assertions. In case of throws/doesNotThrow we wouldn't have that anymore. Only those exceptions should cause a fatal failure in Expect which are not related to the expected error class, i.e. broken code inside the callback.

Comment 30

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

(In reply to comment #29)
> Personally I do not like the way how you i.e. degrade the Expect class in the
> test_assertions.js module by resetting the _logFail method:
> 
> expect._logFail = function() {};

That also affects the following code:

+    if(test.result.throws) {
+      assert.throws(function() {
+        expect[test.fun].apply(expect, test.params);
+      }, test.result.throws, message);
+    }
+    else
+      assert.equal(expect[test.fun].apply(expect, test.params)
+      , test.result, message);

In the else case we do not want to assert. All tests in this file should have an expect check around them to make sure we really execute all tests and do not exit too early if we have a fatal failure.

Comment 31

[Heather Arthur [:harth]]

Attachment: patch to address Henrik's comments

Thanks for clearing this up for me Henrik. Here's a patch with some fixes for what you've mentioned:

* names for anonymous throws() functions
* expose Expect/Assert objects
* expect.throws() doesn't re-throw exception
* use softExpect when testing expect.

Comment 32

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to comment #28)
> (In reply to comment #27)
> > > > * assertion.js should export the Expect and Assert class and not instances of
> > > > both classes.
> > > 
> > > I don't see how it makes a difference, there's no state stored in either class.
> > 
> > There will be no way to work with those classes in a good way, i.e. creating
> > sub classes.
> 
> We can export both the class and instances if you want, but I'm hesitant about
> why you'd want to subclass it. Anything you think you'd want on the assert
> module we should try to get in the core code, and we probably don't want a
> bunch of superfluous assert methods in a shared module.

I'm not 100% sure I agree with you here. There might be assertions that are fairly firefox-specific that come up a lot. It could be useful to add those.

But this being Javascript, we could just add them straight to the assert/expect singletons; a subclass may be clumsier. Ultimately, I don't really care if you export singletons or classes, though, since for us it'll be encapsulated in init.testModule().

> > Also I do not understand why we need the assert.throws around an expect call:
> > 
> >       assert.throws(function() {
> >         expect[test.fun].apply(expect, test.params);
> >       }, test.result.throws, message);
> > 
> > Expect should never throw an error if the assertion condition hasn't been met.
> > The following code has to add a failure only and should not re-throw the Error. 
> > 
> > expect.throws(function () { throw new Error(); }, Error);
> > 
> > So why we would have to enclose it into assert.throws?
> 
> I'm fine with either way, Geo, do you have any opinions on this?

I'm having problems tracking what "this" is given the flurry of replies, but I'll give a stab. 

Henrik's point that expect.throws() shouldn't rethrow an expected exception is right, I think. I believe the node.js version also eats those, plus it'd make testing for the exception rather useless.

I do think that expect.throws() should rethrow -unexpected- exceptions. I think node.js logs a FAIL then rethrows in this case, which makes sense (you didn't get the one you expected, you got a different one instead).

If we implemented the any-exception version of this (i.e. no class provided) I'd expect it to PASS on any exception, without rethrow.

There's also the question of when expect.doesNotThrow() should rethrow. I could see either one of two behaviors being valid:

A) It rethrows any exception encountered, because none is expected. So, it's either PASS or abort.

B) For the named exception, it FAILs without rethrow, but rethrows any other exception. If no exception class is named, it FAILs on any exception, without rethrow. That'd be similar to my expected behavior for expect.throw() above.

In this case, I can't remember what the node.js behavior was; but I think they rethrew any exception, but logged FAILs as in B) first.

And all said, I see you've done a final patch, which I'll also check out. But I wanted to separately put my opinions on behavior in the bug.

Comment 33

[Heather Arthur [:harth]]

(In reply to comment #32)
> But this being Javascript, we could just add them straight to the assert/expect
> singletons; a subclass may be clumsier. Ultimately, I don't really care if you
> export singletons or classes, though, since for us it'll be encapsulated in
> init.testModule().

Ended up exposing both Assert and assert for now.


> Henrik's point that expect.throws() shouldn't rethrow an expected exception is
> right, I think. I believe the node.js version also eats those, plus it'd make
> testing for the exception rather useless.
> 
> I do think that expect.throws() should rethrow -unexpected- exceptions. I think
> node.js logs a FAIL then rethrows in this case, which makes sense (you didn't
> get the one you expected, you got a different one instead).

Henrik is saying Expect shouldn't re-throw an unexpected exception. Assert does rethrow a unexpected exception, and doesn't rethrow an expected one. Node.js doesn't have an "expect" module, just an "assert" module that throws AssertionErrors, so we can't take cue from them here.

> 
> If we implemented the any-exception version of this (i.e. no class provided)
> I'd expect it to PASS on any exception, without rethrow.
> 
> There's also the question of when expect.doesNotThrow() should rethrow. I could
> see either one of two behaviors being valid:
> 
> A) It rethrows any exception encountered, because none is expected. So, it's
> either PASS or abort.
> 
> B) For the named exception, it FAILs without rethrow, but rethrows any other
> exception. If no exception class is named, it FAILs on any exception, without
> rethrow. That'd be similar to my expected behavior for expect.throw() above.

B is close to our current node-based implementation, except We/CommonJS rethrow any exception when there is no named exception.

I think its wise to stay as close as possible to the CommonJS standard.

Comment 34

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to comment #33)

> Henrik is saying Expect shouldn't re-throw an unexpected exception. Assert does
> rethrow a unexpected exception, and doesn't rethrow an expected one. Node.js
> doesn't have an "expect" module, just an "assert" module that throws
> AssertionErrors, so we can't take cue from them here.

Unexpected is unexpected, and should always produce an exception IMO. It's true Expect shouldn't cause an exception -for failing the test- because that's an expected condition, but it certainly should re-raise if the callback itself has a bug in it. 

It's analogous to sending in a numeric first param for expect.match()--you'll still get a TypeError from the internal call to Number.match(), because you caused an unexpected error by not sending a string. That shouldn't get eaten just because it's Expect.

So, I guess I disagree with Henrik here.

> B is close to our current node-based implementation, except We/CommonJS rethrow
> any exception when there is no named exception.
> 
> I think its wise to stay as close as possible to the CommonJS standard.

I'm generally for staying on standards. In this case, I'm not sure I agree with the standard, but if it's working for them...

Comment 35

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

(In reply to comment #34)
> It's analogous to sending in a numeric first param for expect.match()--you'll
> still get a TypeError from the internal call to Number.match(), because you
> caused an unexpected error by not sending a string. That shouldn't get eaten
> just because it's Expect.
> 
> So, I guess I disagree with Henrik here.

I haven't said anything else. But looks like it was not clear enough.

Comment 36

[Heather Arthur [:harth]]

(In reply to comment #34)
> 
> It's analogous to sending in a numeric first param for expect.match()--you'll
> still get a TypeError from the internal call to Number.match(), because you
> caused an unexpected error by not sending a string. That shouldn't get eaten
> just because it's Expect.
> 
> So, I guess I disagree with Henrik here.

True, I agree then that expect.throws() should throw if the block throws an unexpected error.

> 
> > B is close to our current node-based implementation, except We/CommonJS rethrow
> > any exception when there is no named exception.
> > 
> > I think its wise to stay as close as possible to the CommonJS standard.
> 
> I'm generally for staying on standards. In this case, I'm not sure I agree with
> the standard, but if it's working for them...

As long as you know which error it's not supposed to throw, then you're fine. And I think if you're going to be using assert.doesNotThrow() you'll probably know which error you're looking for.

Comment 37

[Heather Arthur [:harth]]

Attachment: follow-up patch to Assert/Expect

This patch addresses Henrik's concerns, except for the one about the expect.throws() functionality. If an unexpected exception occurs, it is re-thrown regardless of whether its in expect or assert.

Comment 38

[cmtalbert]

Comment on attachment 524317 [details] [diff] [review]
follow-up patch to Assert/Expect

(In reply to comment #37)
> Created attachment 524317 [details] [diff] [review]
> follow-up patch to Assert/Expect
> 
> This patch addresses Henrik's concerns, except for the one about the
> expect.throws() functionality. If an unexpected exception occurs, it is
> re-thrown regardless of whether its in expect or assert.
I think that is the proper thing to do.  r+

Comment 39

[Heather Arthur [:harth]]

https://github.com/mozautomation/mozmill/commit/924ae27401c0bdf70b68b75671be2a7951f81840

Comment 40

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

The parameters of the new functions are still not well documented according to what we discussed 2 weeks ago. See the following code how it should look like for JSDoc:

https://github.com/geoelectric/mozmill-api-refactor/blob/165a7b67ea37fb645ccee59c5e68508755057fe1/lib/api/assertions.js#L306