moll / js-must

An assertion library for JavaScript and Node.js with a friendly BDD syntax (awesome.must.be.true()). It ships with many expressive matchers and is test runner and framework agnostic. Follows RFC 2119 with its use of MUST. Good stuff and well tested.
Other
336 stars 35 forks source link

Enable demand(function(){.....}).throw() scenarios when ctx is necessary #5

Closed koresar closed 10 years ago

koresar commented 10 years ago

Examine the following scenario:

function MyClass() {
  this.variable = 'not initialized';
  function myFunction() {
    if (this.variable === 'not initialized') {
      throw new Error('Tried using uninitialized MyClass');
    }
  }
}

var myObject = new MyClass();
myObject.myFunction.must.throw(Error, /uninitialized/);

Clearly, the exception containing 'uninitialized' must be thrown, but instead I get different exception saying "null doesn't have 'variable' property". That is because this.actual.call(null) uses null as the context.

By passing this.actual as the context the fix enables the following testing syntax:

var myObject = new MyClass();
demand(function() { myObject.myFunction() }).must.throw(Error, /uninitialized/);
moll commented 10 years ago

Hey!

Thank you for caring! Fortunately or unfortunately this is a property of JavaScript that when you take the function off of an object myObject.myFunction it won't be tied to myObject any longer. Function context is in this context only a by-product of calling a function on an object and not inherent in the function.

this.actual will be the function itself. Passing itself as the context to itself shouldn't do much. :)

Anyways, I'm guessing what should solve your attempt is using Function.prototype.bind:

myObject.myFunction.bind(myObject).must.throw(...)

I've also now added a small explanation to Must.prototype.throw's documentation. Do you think the phrasing I used would help others in the future?

Let me know if that works for you!

koresar commented 10 years ago

Passing itself as the context to itself shouldn't do much. :)

My findings are different. The pull request works great for me using the syntax I showed. Surprisingly the this.actual context is the one the original function belongs to.

Phrasing is much better in this case demand(function (){myObject.myFunction()}).throw(...) than in this case myObject.myFunction.bind(myObject).must.throw(...)

Please also consider, that following would work as well:

demand( function() { myFactory(myClass).mixin({foo: 'bar'}).myFunction() }).throw(...);

Whereas with the syntax you have suggested I would need to have two lines of code and additional variable:

var myObject = myFactory(myClass).mixin({foo: 'bar'});
myObject.myFunction.bind(myObject).must.throw(...);

Test the change. :) What would be your findings?

moll commented 10 years ago

Oh, but you definitely should be able to use the demand style now, too, as the context of the surrounding function (I named it wrap below) shouldn't have any effect on what goes inside it:

demand(function wrap() { myFactory(myClass).mixin({foo: 'bar'}).myFunction() }).to.throw(...)

Or am I still misunderstanding something?

koresar commented 10 years ago

Ha! Silly me. Sorry. :) Disregard this pull request, please.

moll commented 10 years ago

No prob. Happens to the best of us. ;-)