Merge branch 'bugfix-non-fn-args' of https://github.com/voithos/jasmine into voithos-bugfix-non-fn-args

- Merges #1222 from @voithos
- Fixes #1004

lib/jasmine-core/jasmine.js

@@ -161,7 +161,11 @@ getJasmineRequireObj().base = function(j$, jasmineGlobal) {
   };
 
   j$.isA_ = function(typeName, value) {
-    return Object.prototype.toString.apply(value) === '[object ' + typeName + ']';
+    return j$.getType_(value) === '[object ' + typeName + ']';
+  };
+
+  j$.getType_ = function(value) {
+    return Object.prototype.toString.apply(value);
   };
 
   j$.isDomNode = function(obj) {
@@ -829,6 +833,12 @@ getJasmineRequireObj().Env = function(j$) {
       return spyRegistry.spyOnProperty.apply(spyRegistry, arguments);
     };
 
+    var ensureIsFunction = function(fn, caller) {
+      if (!j$.isFunction_(fn)) {
+        throw new Error(caller + ' expects a function argument; received ' + j$.getType_(fn));
+      }
+    };
+
     var suiteFactory = function(description) {
       var suite = new j$.Suite({
         env: self,
@@ -844,6 +854,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.describe = function(description, specDefinitions) {
+      ensureIsFunction(specDefinitions, 'describe');
       var suite = suiteFactory(description);
       if (specDefinitions.length > 0) {
         throw new Error('describe does not expect any arguments');
@@ -856,6 +867,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.xdescribe = function(description, specDefinitions) {
+      ensureIsFunction(specDefinitions, 'xdescribe');
       var suite = suiteFactory(description);
       suite.pend();
       addSpecsToSuite(suite, specDefinitions);
@@ -865,6 +877,7 @@ getJasmineRequireObj().Env = function(j$) {
     var focusedRunnables = [];
 
     this.fdescribe = function(description, specDefinitions) {
+      ensureIsFunction(specDefinitions, 'fdescribe');
       var suite = suiteFactory(description);
       suite.isFocused = true;
 
@@ -961,6 +974,11 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.it = function(description, fn, timeout) {
+      // it() sometimes doesn't have a fn argument, so only check the type if
+      // it's given.
+      if (arguments.length > 1) {
+        ensureIsFunction(fn, 'it');
+      }
       var spec = specFactory(description, fn, currentDeclarationSuite, timeout);
       if (currentDeclarationSuite.markedPending) {
         spec.pend();
@@ -969,13 +987,19 @@ getJasmineRequireObj().Env = function(j$) {
       return spec;
     };
 
-    this.xit = function() {
+    this.xit = function(description, fn, timeout) {
+      // xit(), like it(), doesn't always have a fn argument, so only check the
+      // type when needed.
+      if (arguments.length > 1) {
+        ensureIsFunction(fn, 'xit');
+      }
       var spec = this.it.apply(this, arguments);
       spec.pend('Temporarily disabled with xit');
       return spec;
     };
 
     this.fit = function(description, fn, timeout){
+      ensureIsFunction(fn, 'fit');
       var spec = specFactory(description, fn, currentDeclarationSuite, timeout);
       currentDeclarationSuite.addChild(spec);
       focusedRunnables.push(spec.id);
@@ -992,6 +1016,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.beforeEach = function(beforeEachFunction, timeout) {
+      ensureIsFunction(beforeEachFunction, 'beforeEach');
       currentDeclarationSuite.beforeEach({
         fn: beforeEachFunction,
         timeout: function() { return timeout || j$.DEFAULT_TIMEOUT_INTERVAL; }
@@ -999,6 +1024,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.beforeAll = function(beforeAllFunction, timeout) {
+      ensureIsFunction(beforeAllFunction, 'beforeAll');
       currentDeclarationSuite.beforeAll({
         fn: beforeAllFunction,
         timeout: function() { return timeout || j$.DEFAULT_TIMEOUT_INTERVAL; }
@@ -1006,6 +1032,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.afterEach = function(afterEachFunction, timeout) {
+      ensureIsFunction(afterEachFunction, 'afterEach');
       currentDeclarationSuite.afterEach({
         fn: afterEachFunction,
         timeout: function() { return timeout || j$.DEFAULT_TIMEOUT_INTERVAL; }
@@ -1013,6 +1040,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.afterAll = function(afterAllFunction, timeout) {
+      ensureIsFunction(afterAllFunction, 'afterAll');
       currentDeclarationSuite.afterAll({
         fn: afterAllFunction,
         timeout: function() { return timeout || j$.DEFAULT_TIMEOUT_INTERVAL; }

spec/core/EnvSpec.js

@@ -26,15 +26,6 @@ describe("Env", function() {
     });
   });
 
-  describe('#describe', function () {
-    var spec = function(done){};
-    it("throws the error", function() {
-      expect(function() {
-        env.describe('done method', spec);
-      }).toThrow(new Error('describe does not expect any arguments'));
-    });
-  });
-
   it('can configure specs to throw errors on expectation failures', function() {
     env.throwOnExpectationFailure(true);
 
@@ -55,14 +46,113 @@ describe("Env", function() {
     }));
   });
 
+  describe('#describe', function () {
+    it("throws an error when given arguments", function() {
+      expect(function() {
+        env.describe('done method', function(done) {});
+      }).toThrowError('describe does not expect any arguments');
+    });
+
+    it('throws an error when it receives a non-fn argument', function() {
+      // Some versions of PhantomJS return [object DOMWindow] when
+      // Object.prototype.toString.apply is called with `undefined` or `null`.
+      // In a similar fashion, IE8 gives [object Object] for both `undefined`
+      // and `null`. We mostly just want these tests to check that using
+      // anything other than a function throws an error.
+      expect(function() {
+        env.describe('undefined arg', undefined);
+      }).toThrowError(/describe expects a function argument; received \[object (Undefined|DOMWindow|Object)\]/);
+      expect(function() {
+        env.describe('null arg', null);
+      }).toThrowError(/describe expects a function argument; received \[object (Null|DOMWindow|Object)\]/);
+
+      expect(function() {
+        env.describe('array arg', []);
+      }).toThrowError('describe expects a function argument; received [object Array]');
+      expect(function() {
+        env.describe('object arg', {});
+      }).toThrowError('describe expects a function argument; received [object Object]');
+
+      expect(function() {
+        env.describe('fn arg', function() {});
+      }).not.toThrowError('describe expects a function argument; received [object Function]');
+    });
+  });
+
+  describe('#it', function () {
+    it('throws an error when it receives a non-fn argument', function() {
+      expect(function() {
+        env.it('undefined arg', undefined);
+      }).toThrowError(/it expects a function argument; received \[object (Undefined|DOMWindow|Object)\]/);
+    });
+
+    it('does not throw when it is not given a fn argument', function() {
+      expect(function() {
+        env.it('pending spec');
+      }).not.toThrow();
+    });
+  });
+
   describe('#xit', function() {
     it('calls spec.pend with "Temporarily disabled with xit"', function() {
       var pendSpy = jasmine.createSpy();
       spyOn(env, 'it').and.returnValue({
         pend: pendSpy
       });
-      env.xit();
+      env.xit('foo', function() {});
       expect(pendSpy).toHaveBeenCalledWith('Temporarily disabled with xit');
     });
+
+    it('throws an error when it receives a non-fn argument', function() {
+      expect(function() {
+        env.xit('undefined arg', undefined);
+      }).toThrowError(/xit expects a function argument; received \[object (Undefined|DOMWindow|Object)\]/);
+    });
+
+    it('does not throw when it is not given a fn argument', function() {
+      expect(function() {
+        env.xit('pending spec');
+      }).not.toThrow();
+    });
+  });
+
+  describe('#fit', function () {
+    it('throws an error when it receives a non-fn argument', function() {
+      expect(function() {
+        env.fit('undefined arg', undefined);
+      }).toThrowError(/fit expects a function argument; received \[object (Undefined|DOMWindow|Object)\]/);
+    });
+  });
+
+  describe('#beforeEach', function () {
+    it('throws an error when it receives a non-fn argument', function() {
+      expect(function() {
+        env.beforeEach(undefined);
+      }).toThrowError(/beforeEach expects a function argument; received \[object (Undefined|DOMWindow|Object)\]/);
+    });
+  });
+
+  describe('#beforeAll', function () {
+    it('throws an error when it receives a non-fn argument', function() {
+      expect(function() {
+        env.beforeAll(undefined);
+      }).toThrowError(/beforeAll expects a function argument; received \[object (Undefined|DOMWindow|Object)\]/);
+    });
+  });
+
+  describe('#afterEach', function () {
+    it('throws an error when it receives a non-fn argument', function() {
+      expect(function() {
+        env.afterEach(undefined);
+      }).toThrowError(/afterEach expects a function argument; received \[object (Undefined|DOMWindow|Object)\]/);
+    });
+  });
+
+  describe('#afterAll', function () {
+    it('throws an error when it receives a non-fn argument', function() {
+      expect(function() {
+        env.afterAll(undefined);
+      }).toThrowError(/afterAll expects a function argument; received \[object (Undefined|DOMWindow|Object)\]/);
+    });
   });
 });

src/core/Env.js

@@ -292,6 +292,12 @@ getJasmineRequireObj().Env = function(j$) {
       return spyRegistry.spyOnProperty.apply(spyRegistry, arguments);
     };
 
+    var ensureIsFunction = function(fn, caller) {
+      if (!j$.isFunction_(fn)) {
+        throw new Error(caller + ' expects a function argument; received ' + j$.getType_(fn));
+      }
+    };
+
     var suiteFactory = function(description) {
       var suite = new j$.Suite({
         env: self,
@@ -307,6 +313,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.describe = function(description, specDefinitions) {
+      ensureIsFunction(specDefinitions, 'describe');
       var suite = suiteFactory(description);
       if (specDefinitions.length > 0) {
         throw new Error('describe does not expect any arguments');
@@ -319,6 +326,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.xdescribe = function(description, specDefinitions) {
+      ensureIsFunction(specDefinitions, 'xdescribe');
       var suite = suiteFactory(description);
       suite.pend();
       addSpecsToSuite(suite, specDefinitions);
@@ -328,6 +336,7 @@ getJasmineRequireObj().Env = function(j$) {
     var focusedRunnables = [];
 
     this.fdescribe = function(description, specDefinitions) {
+      ensureIsFunction(specDefinitions, 'fdescribe');
       var suite = suiteFactory(description);
       suite.isFocused = true;
 
@@ -424,6 +433,11 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.it = function(description, fn, timeout) {
+      // it() sometimes doesn't have a fn argument, so only check the type if
+      // it's given.
+      if (arguments.length > 1) {
+        ensureIsFunction(fn, 'it');
+      }
       var spec = specFactory(description, fn, currentDeclarationSuite, timeout);
       if (currentDeclarationSuite.markedPending) {
         spec.pend();
@@ -432,13 +446,19 @@ getJasmineRequireObj().Env = function(j$) {
       return spec;
     };
 
-    this.xit = function() {
+    this.xit = function(description, fn, timeout) {
+      // xit(), like it(), doesn't always have a fn argument, so only check the
+      // type when needed.
+      if (arguments.length > 1) {
+        ensureIsFunction(fn, 'xit');
+      }
       var spec = this.it.apply(this, arguments);
       spec.pend('Temporarily disabled with xit');
       return spec;
     };
 
     this.fit = function(description, fn, timeout){
+      ensureIsFunction(fn, 'fit');
       var spec = specFactory(description, fn, currentDeclarationSuite, timeout);
       currentDeclarationSuite.addChild(spec);
       focusedRunnables.push(spec.id);
@@ -455,6 +475,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.beforeEach = function(beforeEachFunction, timeout) {
+      ensureIsFunction(beforeEachFunction, 'beforeEach');
       currentDeclarationSuite.beforeEach({
         fn: beforeEachFunction,
         timeout: function() { return timeout || j$.DEFAULT_TIMEOUT_INTERVAL; }
@@ -462,6 +483,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.beforeAll = function(beforeAllFunction, timeout) {
+      ensureIsFunction(beforeAllFunction, 'beforeAll');
       currentDeclarationSuite.beforeAll({
         fn: beforeAllFunction,
         timeout: function() { return timeout || j$.DEFAULT_TIMEOUT_INTERVAL; }
@@ -469,6 +491,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.afterEach = function(afterEachFunction, timeout) {
+      ensureIsFunction(afterEachFunction, 'afterEach');
       currentDeclarationSuite.afterEach({
         fn: afterEachFunction,
         timeout: function() { return timeout || j$.DEFAULT_TIMEOUT_INTERVAL; }
@@ -476,6 +499,7 @@ getJasmineRequireObj().Env = function(j$) {
     };
 
     this.afterAll = function(afterAllFunction, timeout) {
+      ensureIsFunction(afterAllFunction, 'afterAll');
       currentDeclarationSuite.afterAll({
         fn: afterAllFunction,
         timeout: function() { return timeout || j$.DEFAULT_TIMEOUT_INTERVAL; }

src/core/base.js

@@ -38,7 +38,11 @@ getJasmineRequireObj().base = function(j$, jasmineGlobal) {
   };
 
   j$.isA_ = function(typeName, value) {
-    return Object.prototype.toString.apply(value) === '[object ' + typeName + ']';
+    return j$.getType_(value) === '[object ' + typeName + ']';
+  };
+
+  j$.getType_ = function(value) {
+    return Object.prototype.toString.apply(value);
   };
 
   j$.isDomNode = function(obj) {