tj / should.js

BDD style assertions for node.js -- test framework agnostic
MIT License
2.75k stars 195 forks source link

Returning original should call in error object #180

Closed renehamburger closed 10 years ago

renehamburger commented 10 years ago

The way that we use should, we often run into issues with AssertionError messages not being specific enough to distinguish several should calls within the same test.

The changes suggested here add the original should call as a string to the AssertionError object .message and as a separate .call property. It requires the original call to be wrapped in an anonymous self-invoking function, which is easily done in CoffeeScript: do -> a.should.eql(b).

The original call is then printed out with the error message and, with a small change in mocha's 'reporters/base.js' on Line 356: + (err.call ? err.call + '\n\n' : ''), also by mocha's inline diff.

If you're interested in merging this feature in, I can add a couple of tests.

btd commented 10 years ago

If you are trying to add name of assertion it can be done in Assertion.add, with arguments as they was passed in test sources, it is harder. But i do not think that wrap every call in function it is an option (not everyone uses cs).

renehamburger commented 10 years ago

Yes, we could add a name or description for the assertion, but that would be a lot of additional code if we did that for every assertion.

I agree that the function wrapper is a bit cumbersome in JavaScript, so I don't expect many or any JavaScript users to make use of this feature. But for CoffeeScript users this would be a great feature. I think that nothing is as descriptive as the actual assertion code when it comes to quickly seeing why a test failed.

renehamburger commented 10 years ago

Another reason for providing this feature for CoffeeScript is that the line numbers in the stack trace for failed assertion refer to the compiled js files, so it's always a step more in CS to figure out which assertion failed.

btd commented 10 years ago

So summing:

  1. This makes sense only for CS
  2. Mocha do not support output .call, and you will need to either create PR (but it will not be accepted as it is CS + should.js specific) or create you own.
  3. This util.getCall looks weird and uses deprecated .caller.
  4. It is not responsibility of assertion library to show sources.

I will not accept it. For myself better for all do 2 PR for mocha, one to support source maps for mocha and second to show lines of tests which failed.

renehamburger commented 10 years ago

OK, this is your decision, but let me just clarify a few points:

  1. Yes, it would be very easy to use it in CS, but if a JS user of should was struggling with figuring out which one of many similar assertions failed, he might well find that wrapping it in an anonymous function call would be worth it.
  2. I wasn't using the latest version of Mocha when I suggested a pull request is needed. The latest version of Mocha always prints out the AssertionError message and the caller is appended to that message. So no PR needed.
  3. Are you referring to the regular expression? I added it to make sure that the caller only consists of a single should assertion. Function.caller could be used instead (see Stackoverflow Comment and MDN).
  4. I think an assertion library should show the assertion that failed if possible. At the moment you're printing out expected true to equal false. But wouldn't it be much better to add the original assertion res.body.something.should.eql(false) to this??
rudolf commented 10 years ago

I agree with most of your points. The syntax and implementation makes it cumbersome to use.

Regarding (4), I do think assertion libraries should present as much information as possible in absence of a message being passed to the assertion.

Wikipedia is by no means authoritative, but I think many developers expect similar results from their assertion library: "Since an assertion failure usually reports the code location, one can often pin-point the error without further debugging"

Another example is the pytest library which has even more powerful assertion introspection.

If we go instead with an implementation similar to wish assertion library, I believe we could get all of the benefits of better output without uglyness of anonymous function call wrappers.

Would you consider a PR based on this?