Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require arguments to beforeEach, it, etc, to be actual functions #1222

Merged
merged 1 commit into from
Feb 15, 2017

Conversation

voithos
Copy link
Contributor

@voithos voithos commented Oct 18, 2016

This fixes #1004. Let me know if there are any concerns or ideas for improvement! Note that this does change the public API in a few ways, namely:

  • Calling describe, beforeEach, et al, without providing a function argument will now throw an error instead of silently skipping tests (this is the goal).
  • For the edge cases of xdescribe and xit (since the suite/spec isn't actually ran), I chose to disallow non-function arguments if they are given (that is, pending specs that omit the fn argument completely, like it('foo'), still work as intended).

JSHint is green, tests pass in Node and browser environments (haven't tested IE yet).

@slackersoft
Copy link
Member

Thanks for taking a shot at this. Unfortunately, you have introduced a breaking change that we don't want. An it without a function argument ha special meaning in Jasmine, it is a quick way to have a pending test that will be implemented later. The rest of the methods don't make sense without a function argument though.

Please update this PR so that an it with no function is still pending.

Thanks for using Jasmine!

@@ -288,6 +288,13 @@ getJasmineRequireObj().Env = function(j$) {
return spyRegistry.spyOn.apply(spyRegistry, arguments);
};

var ensureIsFunction = function(fn, caller) {
var fnType = Object.prototype.toString.call(fn);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to just use j$.isFunction_ here instead of writing it again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks. I think it'd be valuable to include the type of the (invalid) argument, so I extract the Object.prototype.toString call out to a helper, j$.getType_.

@voithos
Copy link
Contributor Author

voithos commented Oct 20, 2016

Thanks for the review! I modified it to allow an it or xit without a function argument (I felt that xit made sense - and in fact, there was already a spec that used xit without an argument. If a function argument is given, however, the current code will still check that it is of the correct type). Let me know if there are other changes you'd like to see.

@voithos
Copy link
Contributor Author

voithos commented Oct 25, 2016

@slackersoft Do you have any more feedback on this PR? Anything I should change?

@voithos
Copy link
Contributor Author

voithos commented Nov 5, 2016

@slackersoft Friendly ping?

@slackersoft
Copy link
Member

This looks pretty good now, but I just ran the specs in IE (they don't run for pull requests on travis due to secure environment variables) and I'm seeing a bunch of errors for IE8 that look like:

Failures:
          Env #beforeEach throws an error when it receives a non-fn argument

          Expected function to throw an exception with a message matching /beforeEach expects a function argument; received \[object (Undefined|DOMWindow)\]/, but it threw an exception with message 'beforeEach expects a function argument; received [object Object]'.
          No stack trace present.

So it looks like IE (at least in version 8) isn't getting the type information we expect.

@voithos
Copy link
Contributor Author

voithos commented Dec 28, 2016

I took a look, and indeed, it seems like IE8 returns [object Object] whenever Object.prototype.toString.apply is called with either undefined or null. Thankfully, though, it seems like it returns the correct thing for functions ([object Function]), so I believe that the actual type logic will still work out (it's just that the error message will be slightly unclear - but that's still better than no error message!).

I modified the specs to also accept [object Object] in the thrown exception message, which should allow the specs to be run on IE8 (although I haven't had a chance to actually do so, since I don't have easy access to IE8). Let me know if anything else isn't working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test results not reported when teardown is undefined
2 participants