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

Async test "done" callbacks should take an optional error in Jasmine 2.0 #423

Closed
Eric-Wright opened this issue Aug 28, 2013 · 5 comments
Closed

Comments

@Eric-Wright
Copy link

I really like that Jasmine 2.0 has adopted a Mocha-like done() callback for completing async tests - they're much easier to write now. Mocha's done() callback also accepts an optional error parameter; if omitted, the test succeeds, otherwise the test fails by throwing the error. I propose that Jasmine adds the same support to make it easier to write async tests.

An example of how to use this feature:

it("should save without error", function(done) {
  var user = new User('Eric');
  user.save()
    .done(function() { done(); }
    .fail(function() { done("save error"); });
});

Without this support, async test failures are a little more cumbersome to write. In this example, I have to force a test failure in my callback function before calling done():

it("should save without error", function(done) {
  var user = new User('Eric');
  user.save()
    .done(function() { done(); }
    .fail(function() {
      expect("save error").toBe(undefined); // something to cause the test to fail
      done();
     });
});

I modified Spec.js to support this in my local copy of Jasmine. I can submit a pull request you're interested.

@infews
Copy link
Contributor

infews commented Aug 28, 2013

In this use case, where's the expectation to show the failure?

@Eric-Wright
Copy link
Author

If I understand your question, in my implementation, I put the expectation inside of callDone in Spec.js:

        var callDone = function(err) {
          Function.prototype.apply.apply(self.timer.clearTimeout, [j$.getGlobal(), [timeout]]);
            // BEGIN: new code for handling optional err param
            if (err) {
              expect(function() {
                throw err;
              }).not.toThrow();
            }
            // END: new code
            done();
        };

With this implementation, the example above would show "Expected function not to throw, but it threw 'save error'." in the test report.

@infews
Copy link
Contributor

infews commented Aug 28, 2013

We're not fans of that idea. Expectations should be very explicitly inside your it - that's in the spirit of keeping your tests as better explicit documentation of how the code works.

From a Jasmine implementation perspective, having Spec know about expect violates some of our abstraction rules in the 2.0 codebase.

We want to encourage specs of the more wordy example. And yet, what are you really trying to test in that example?

@Eric-Wright
Copy link
Author

I see. Purely from an implementation perspective, to fix the abstraction rule violation, I could replace the expect call with an onException call, just like the timeout code does:

        var timeout = Function.prototype.apply.apply(self.timer.setTimeout, [j$.getGlobal(), [function() {
          onException(new Error('timeout'));
          done();
        }, 10000]]);

        var callDone = function(err) {
          Function.prototype.apply.apply(self.timer.clearTimeout, [j$.getGlobal(), [timeout]]);
          if (err) {
            onException(new Error(err));
          }
          done();
        };

But from what I understand you saying, that's not the Jasmine way since the expectation isn't written out explicitly in test code.

What we're trying to test is that our async code follows expected paths. We use jQuery promises heavily, so in each instance, we expect that .done() was called and .fail() was not (or vice-versa if we're doing negative case testing). Here's one way I could write out those expectations:

it("should save without error", function(done) {
  var successCallback = jasmine.createSpy('successCallback');
  var failCallback = jasmine.createSpy('failCallback');
  var user = new User('Eric');
  user.save()
    .done(successCallback)
    .fail(failCallback)
    .always(function() { 
      expect(successCallback).toHaveBeenCalled();
      expect(failCallback).not.toHaveBeenCalled();
      // ... and other expect() calls as needed to check object state
      done();
    });
});

For a few tests, this is OK. But we have over 60 async tests like this currently, and writing more every day. So, we're looking for ways to make our async test code easy to read and write, and the first example I posted is one approach.

Do you have any recommendations on how we should approach this problem? Thanks for your consideration.

@infews
Copy link
Contributor

infews commented Aug 29, 2013

I don't know enough about your code or the particular promise implementation. But it seems that you're executing a lot of code in order to test the callback paths.

I'd consider maybe only testing the success callback in a valid case and the failing case in the invalid save. Pairing those two tests together will make the suite easier to read and still give you sufficient coverage.

I'm also a fan of not over DRYing specs. Yes, it may seem tedious. But remember that you're typing for the future you to understand the test suite, not just the you that is tired of typing today.

Said from a different take, @ragaskar is always reminding me that a painful test suite is trying to tell you something - usually that your objects are factored wrong.

Let's take this over to the Jasmine mailing list and have more people contribute tips.

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

No branches or pull requests

2 participants