unexpectedjs / unexpected

Unexpected - the extensible BDD assertion toolkit
http://unexpected.js.org/
MIT License
371 stars 29 forks source link

'to satisfy' assertion and expect.fn #40

Closed papandreou closed 10 years ago

papandreou commented 10 years ago

We've identified a need for an assertion similar to to have properties (and a bit like to equal with objects), but which offers the ability to "step out of" the equality mode by specifing a named assertion or a function that decides whether a value buried somewhere inside an object meets the criteria. We've discussed this quite a bit before: #24, #27, #31, #36. I think we should close the other issues (maybe except #31) and come up with a spec here.

Last time we talked about it, we came up with something like this:

expect({foo: 'bar'}, 'to satisfy', {
    foo: expect.fn('to be a string')
});
expect({foo: [123]}, 'to satisfy', {
    foo: expect.fn('to be an array whose items satisfy', 'to be a number')
});
expect({foo: [123]}, 'to satisfy', {
    foo: [expect.fn('to be a number')]
});

We also found out that it would be a great bonus if the to satisfy assertion could have exactly the same semantics as the RHS of the to be an array whose items satisfy family of assertions so those could simply delegate to it. That means it'll have to special case strings and functions (not just the output of expect.fn):

expect(123, 'to satisfy', 'to be a number');
expect(123, 'to satisfy', 'to satisfy', 'to satisfy', 'to be a number');
expect({foo: 123}, 'to satisfy', function (obj) {
    expect(obj.foo, 'to equal', 123);
});

For consistency I suppose we should execute all functions in the RHS (not just the ones returned by expect.fn):

expect({foo: 123}, 'to satisfy', {
    foo: function (foo) {
        expect(foo, 'to equal', 123);
    }
);

That'd make it harder to use to satisfy to compare functions, but I suspect that's rarely needed.

The above would be worth implementing in itself. The only unsolved problem seems to be what to show in the diff when the RHS is a function or named assertion and the expectation isn't met. The fact that test frameworks produce JSON/string diffs of expected and actual makes it hard to present this information. We sort of decided to be lazy and do something like this for now:

 {
+    "foo": 123,
-    "foo": {
-        $Error: "expected 123 to be a string"
-    },
     "bar": 456
 }

... but aim for doing something nicer whenever test framework APIs gain the ability to display (annotated) object diffs.

My pet additions to the above

The expect.fn helper could support and/or and thus solve one of the use cases for #31:

expect({foo: 123}, 'to satisfy', {
    foo: expect.fn('to be a number').and('to be greater than', 10)
});

IMO there should also be a way to make sure that the object being matched does not have a specific property.

expect({foo: 123}, 'to satisfy', {
    bar: expect.fn('to be undefined')
});

Also it would be great if there could be a flag to switch into an "exhaustive" mode where the matched object cannot have more properties than the ones in the RHS (like the difference between to have properties and to equal):

expect({foo: 123}, 'to exhaustively satisfy', {
    bar: expect.fn('to be undefined')
});

Then you'd be able to switch modes easily:

expect({foo: {bar: 123}, baz: 456}, 'to satisfy', {
    foo: expect.fn('to exhaustively satisfy', {bar: 123})
});
expect({foo: {bar: 123, baz: 456}}, 'to exhaustively satisfy', {
    foo: expect.fn('to satisfy', {bar: 123})
});

This would compose nicely when delegating to to satisfy from another assertion, eg.:

expect.addAssertion('to be an image whose metadata [exhaustively] satisfies', function (expect, subject, value) {
    expect(getImageInfoFromBuffer(subject), 'to [exhaustively] satisfy', value);
});

// Make sure that the image is a 123x456 GIF that doesn't have an `isAnimated` property:
expect(fs.readFileSync('image.gif'), 'to be an image whose metadata exhaustively satisfies', {
    contentType: 'image/gif',
    width: 123,
    height: 456
});
sunesimonsen commented 10 years ago

@papandreou thank you so much for this very detailed and exact description of all the discussions we had on this subject.

I think you have almost every thing covered. There one thing we talked about and that is how we support equality of custom types. You can see an example of this below, where we are using a custom type for Knockout observables.

expect({
    foo: ko.observable({ baz: 123 }),
    bar: ko.observable(456)
}, 'to satisfy', {
    foo: ko.observable({ baz: expect.fn('to be a number') }),
    bar: ko.observable(456)
});

We talked about making custom types return an unwrapped object that will be used for equality. But thinking a bit more about it, I don't think we have a proper solution for this case yet.

I don't know if this should even be supported.

papandreou commented 10 years ago

Right, I forgot about those custom types again. Yeah, there's the problem of the current equal function on custom types not covering the to match case because those "complex" types explicitly delegate to expect.equal. Seems like an implementation detail at this point, though, as there's no doubt that we want your snippet to work.

Spec-wise it seems to me that all custom types, including those that recognize functions (such as ko.observable instances) should be prioritized above to satisfy's special casing of functions in the RHS object.

papandreou commented 10 years ago

I think I'd like regexps in the RHS object to be matched instead of compared for equality. In AssetGraph's query model that decision has proven very useful. I expect that you'll very rarely need to look for specific values of regexps, and even so, you'd still be able to go:

expect({foo: /abc/}, 'to satisfy', {foo: expect.fn('to equal', /abc/)});
papandreou commented 10 years ago

WIP at https://github.com/sunesimonsen/unexpected/tree/toSatisfy (branched out from https://github.com/sunesimonsen/unexpected/tree/typedAssertions)

I have a pretty good feeling about it. All the examples listed here work, except the and/or thing, which I did not attempt (yet). Managed to get the "whose items satisfy" etc. assertions to delegate to it, which saves a lot of code and gives us things like this for free:

expect(['a', 'aa', 'aaa', 'b'], 'to be an array whose items satisfy', /a+/);
Error: failed expectation in [ 'a', 'aa', 'aaa', 'b' ]:
  3: expected 'b' to satisfy assertion /a+/
[...]

There are still a lot of things that can be improved, especially error messages.

papandreou commented 10 years ago

Loose idea: Rename to satisfy to to match. It's kind of a generalization of the existing to match.

sunesimonsen commented 10 years ago

Nice - you have really been a busy bee this weekend ;-) About to satisfy vs. to match - I like to satisfy better because it is more general, you have a test subject you want to satisfy some kind of a specification. We can consider making to match an alias for to satisfy.

papandreou commented 10 years ago

Well, we need this thing to go out :)

I especially like that only the core variant of to satisfy needs to implement the not flag: https://github.com/sunesimonsen/unexpected/blob/toSatisfy/lib/assertions.js#L471-L474 -- although there might be more idiomatic way of doing that?

The integration with the messy library is now a standalone library built on top of this (and magicdiffs): https://github.com/papandreou/unexpected-messy/blob/master/lib/unexpectedMessy.js

Looks like it all fits together nicely. The only thing that annoys me about to satisfy right now is that I kind of need for messy.HttpRequest and messy.HttpRequest to be able to delegate to the to satisfy registered for their superclass (messy.Message). Kind of an explicit type cast to invoke a lower-priority assertion. It might pop up in other situations as well, but for now I just pulled the handler for messy.Message into a function that the other assertions use Function.prototype.call to invoke on the Assertion instance: https://github.com/papandreou/unexpected-messy/blob/master/lib/unexpectedMessy.js#L19-L35

sunesimonsen commented 10 years ago

This looks very cool and advanced. I don't know how to work around the casting problem, the way you done it is properly the easiest way. It seems we are getting very close to something useful.

sunesimonsen commented 10 years ago

Landed on https://github.com/sunesimonsen/unexpected/tree/v5

papandreou commented 10 years ago

Changed expect.fn to expect.it in ee8cab3