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

Custom error descriptions #14

Closed bajtos closed 9 years ago

bajtos commented 10 years ago

Please add support for custom error descriptions, it makes assertion messages much more useful.

Consider the following code I would write with chai:

it('deletes files', function() {
   var configPath = // resolve
   var cachePath = // resolve

  // this is the method under test, it deletes config and cache
  removeEntry();

   expect(fs.existsSync(configPath), 'config exists').to.be.false();
   expect(fs.existsSync(cachePath), 'cache exists').to.be.false();
});

When I run it using must, the error message is not very helpful:

AssertionError: true must be false
  at removeentry.test.js:8
  etc.

I would like to get enough information to know why the test failed, without the need to look up the line in the source code that executed the assertion

AssertionError: config exists: true must be false
 at removeentry.test.js:8
 etc.

I am happy to submit a pull request if you are willing to accept this feature.

bajtos commented 10 years ago

@moll Any opinions on this? Is the proposed API (second parameter to must function) ok with you?

moll commented 10 years ago

Hey! Thanks for proposing!

I actually think this problem would be better solved by the test runner if it could print the line with the assertion along with the error. Typing the words "config" and "exists" again somewhere later sounds a little onerous. That would also work equally well whether you use the expect/demand style or the fluent style.

I don't yet know of a test runner that would do such a thing automatically. I have thought about creating a test runner some day with better design and intelligent behavior, but won't probably get to it that quickly... :)

bajtos commented 10 years ago

I actually think this problem would be better solved by the test runner if it could print the line with the assertion along with the error.

I don't yet know of a test runner that would do such a thing automatically. I have thought about creating a test runner some day with better design and intelligent behavior, but won't probably get to it that quickly... :)

While it is a nice idea (and I remember there is a test or assertion library that have this already implemented), it won't solve my problem in near future.

Adding custom messages to assertion is a standard feature, it's supported by Node's builtin assert module, chaijs, visionmedia's should, not to mention NUnit, JUnit, and so on.

To be honest, I wanted to use js-must instead of chaijs because both me and my jshint dislike property-based API like expect(foo).to.be.undefined (instead of expect(foo).to.be.undefined()). I am willing to invest some time to implement the features I miss in js-must (#14, #16), but if I can't make your library work for me, I'll simply switch back to chaijs. The workarounds like expect(foo).to.equal(undefined) were bearable so far and I have not run into any blocking issues there.

moll commented 10 years ago

Thanks for clarifying. I've got a couple of Qs more to get a better understanding.

it won't solve my problem in near future.

Did you mean it won't solve your problem in general or just that nothing like it is available right now?

Adding custom messages to assertion is a standard feature, it's supported by Node's builtin assert module, chaijs, visionmedia's should, not to mention NUnit, JUnit, and so on.

It is indeed widely-spread, but that could either be because it's truly necessary or because of cargo culting. The latter is also well spread amongst designers, so that's why I want to understand the use cases fully before moving on. Are the two lines above the most common case you tend to use custom descriptions for?

I appreciate your care and am thankful for it! Don't take my initial resistance to some ideas personally — I simply want to do the right thing for the long run, even if that may go against the status quo. When it comes to this issue, though, I'm still open for discussion. :)

bajtos commented 10 years ago

it won't solve my problem in near future.

Did you mean it won't solve your problem in general or just that nothing like it is available right now?

I want to make the assertion messages produces by my tests very informative, so that you can figure out what went wrong without studying the test code source.

I can accomplish that in chaijs but I cannot do that with js-must. And I don't want to wait until you get time to implement printing of the line where the assertion failed. Which BTW may not work for multi-line statements or helpers wrapping multiple assertions in a single function.

Adding custom messages to assertion is a standard feature, it's supported by Node's builtin assert module, chaijs, visionmedia's should, not to mention NUnit, JUnit, and so on.

It is indeed widely-spread, but that could either be because it's truly necessary or because of cargo culting. The latter is also well spread amongst designers, so that's why I want to understand the use cases fully before moving on. Are the two lines above the most common case you tend to use custom descriptions for?

These are the situation where I use custom descriptions:

TDD purists will say I should split the test to have a single assertion per test and make the test name descriptive enough. While it is a good rule of thumb, there are cases where it's difficult and impractical to apply.

mcdonnelldean commented 9 years ago

Perhaps to add some context around this feature as I would like it also.

I would also like it because it simplifies tests like ensuring all items in a collection have a given field or value. Currently there is no good way to find out which element in a collection failed without resorting to console.log.

So something like:

_.forEach(elements, function(element) {
   expect(element.id, 'Id: ' + element.id).to.be.falsy();
});

While I agree mostly with the one method per assertion rule, dealing with collections tends to mean asserting on each item in said collection.

michaeljones commented 9 years ago

Very happy to have switched to must as I strongly agree with avoiding the undefined issues in chai. I was surprised to find that must doesn't support extra messages though. I've got a similar situation with a false should equal true style error message which I've managed to improve a little by juggling what I am testing but it means there is no room for providing additional contextual information about what inputs lead to a failure as far as I can tell. It means that a test failure is going to immediately require another run with print statements rather than the test assertion being able to provide useful information.

Maybe I'm doing it wrong though? Should I be running all my tests through node-inspector or something similar to debug the results rather than expect to have enough information from the test failure itself?

The example at the top would be helped by the framework printing the failing line but my failing line is:

        const spec = interfaces[module].methods[func];
        const result = complianceChecker.check(spec, data);
        expect(result.message).to.be.undefined()

Which has no visible information that is of use. I kind of need to know the value of spec & data to understand what has gone wrong. I started with expect(result.success).eql(true) but that has almost no information in the output so I've moved to this but the result is still less than desirable as I now get AssertionError: "Missing attribute: id" must be undefined which is only useful if I remember to ignore the must be undefined bit and work with the message I want.

Considering a selling point in your readme is 'Human readable error messages', it might be worth allowing humans to make them more readable when it suits them.

All that said, I've never written an assertion framework before and I'm very grateful for the work you've done! Cheers!

moll commented 9 years ago

Good news, everyone! I'm sold for now. :innocent:

A message argument for demand/expect style invocation is there for you to give a go:

var demand = require("must")
demand(undefined, "The undefined undefineds").be.undefined()

It's up as v0.13.1 and docs at https://github.com/moll/js-must/blob/master/doc/API.md#Must. Shoot a message if it's not behaving as expected.

PS. Should really cut v1 at some point, too. This isn't Google with its indefinite betas after all. ^_^

michaeljones commented 9 years ago

Great news! Thank you!