Closed keithamus closed 6 years ago
As I wrote in #528 and #467, one major problem in my opinion is sharing namespaces, for instance two plugins that define a depth
assertion - for instance, both a SwimmingPool
and a BTree
can have a depth. Currently plugins can use overwriteMethod
to imperatively decide whether they support the asserted object. As soon as #467 is solved, I see no reason for any plugin to use addMethod
, and as agreed with @keithamus in #117 I intend to amend the documentation to reflect that the best practice is to user overwriteMethod
.
Having said that, I think that the new API should allow declaring a predicate for whether this plugin is applicable for the object under assertion:
export default {
assertions: {
'depth': {
'predicate': obj => obj instanceof SwimingPool
'assert': {...}
}
}
}
The intent with the above examples was that interceptors
would handle "overriding" of methods, as demonstrated with the equal
interceptor:
// Interceptors can also override methods without using any flags, instead it can // use ducktyping to toggle an assertion (kind of like one of those Railroad // switches, redirecitng the assertion given a set of requirements). // Here is an example that could be found in a chai-react plugin, that uses a // different deep equality on two React elements 'equal': (assertion, actual, expected) => { if (React.isElement(acutal) && React.isElement(expected)) { return { result: myReactDeepEqualityAlgo(actual, expected), } } else { return assertion(actual, expected); } }
Having said that, I like the idea of having a predicate based assertions - but I wonder if that is perhaps something that becomes difficult to debug:
expect(thingThatMatchesNoPredicates).to.equal(foo)
, does it error saying that the assertion can't be executed? How do we do that in a user friendly way?actual
? or expected
? or `both? I'm afraid that the way you described the API might be a bit too loose, so that people (like I just did!) could miss it altogether!
What happens if you call an assertion for something which has no matching predicates? If I call expect(thingThatMatchesNoPredicates).to.equal(foo), does it error saying that the assertion can't be executed? How do we do that in a user friendly way?
Well, I expect that an exception would be thrown. What happens in the solution for #467?
Presumably if a predicate function is omitted, then that particular assertion can "handle any type" at goes to the bottom of the stack, and asserts with predicates provided go to the top? In that case, what happens when we have two methods which both omit predicates? Is one an invalid plugin?
Good question. What did you imagine in your design? what you're doing, as I understand, is kind of a chain of responsibility pattern. How would it deal with a plugin with no predicate? with a conflict? seems like it's a matter of sorting the plugins, so that the first plugin registered wins.
Presumably the predicate determines viability of actual? or expected? or
both?
actual.
expectedis created in the call site, where the caller is aware of the type of
actualso they shape
expected` accordingly. It is, after all, their expectation :)
Well, I expect that an exception would be thrown. What happens in the solution for #467?
An exception should be thrown - but what kind of exception? How can we surface this information to the user in a way that they would understand without being a plugin author.
Good question. What did you imagine in your design?
The design I have above skirts over this issue by making the author manually override methods. Not saying this is better or worse.
what you're doing, as I understand, is kind of a chain of responsibility pattern. How would it deal with a plugin with no predicate? with a conflict? seems like it's a matter of sorting the plugins, so that the first plugin registered wins.
The problem here is that we're pushing the problem onto the user, meaning more support requests/bug reports, a requirement for better docs for every plugin. It could potentially be a big problem. Ideally we should have a solution which does not depend on fragile things like plugin order.
Eventually this is probably your decision, right? :)
I think that one of the things that really hinders progress here is the fact that expect
and should
are language chaining-oriented. Compare, for instance, to the API and usage of matching frameworks such as hamcrest or Specs2.
In both, additional matchers (plugins) are just functions that are passed to an assertion method, so that the only chance for a conflict would be the user actively importing two matchers with the same name - in which case the file will not compile / run.
Eventually this is probably your decision, right? :)
Hopefully not. I'm not a plugin author, nor a BDFL. I simply have the benefit of being the consumer of all issues meaning I can specify requirements for improving our plugin system. The reason this issue exists is for active bikeshedding by plugin authors so that they're getting the simplest system for writing plugins (that falls inline with some of the major issues chai faces).
In both, additional matchers (plugins) are just functions that are passed to an assertion method, so that the only chance for a conflict would be the user actively importing two matchers with the same name - in which case the file will not compile / run.
This is definitely an option. I have considered looking at enforcing name-spacing of plugins, so that conflicts can't occur, but I've steered away from that for a few reasons:
expect(mySpyThing).to.be.a.spy.and.be.called(...)
or assert.spyCalled(mySpyThing)
is not very pretty, and is extra typing for sometimes no reason.I suppose we could try to implement some kind of trait based system similar to hamcrest, it doesn't differ that much from the predicate system you proposed above, with similar pitfals.
Ultimately everything will have upsides and downsides, our rubric is:
Ok. How do we make sure that all plugin authors know that this issue exists and can put in their 2 cents?
I have, in the past, @mentioned all plugin authors for sweeping chai changes (e.g. for migrating the chai plugins page), but thats kind of the nuclear option.
@vesln and @marcodejongh as you've experienced some problems with the existing plugin architecture (https://github.com/producthunt/chai-enzyme/pull/11) I was hoping I could get your thoughts and opinions on the ideas expressed here.
@keithamus yep! i'm going to go through the proposal this weekend and share feedback
Awesome thanks!
Hey @vesln did you manage to get any time to look at this? I'd be really interested in hearing your feedback.
@electricmonk I think having a predicate
kind of syntax is the right way to go - and pushing everything into assertions may just be the right way to go. I think if a predicate
was required for every assertion, it would solve a lot of the issues. Thoughts?
@keithamus can you please elaborate with a usage example?
@electricmonk it'd be the same as you suggested here but the predicate
key is a required key - and if it isn't present then Chai would throw upon loading the plugin.
I think the way we express predicates could be slightly different though - I think predicate
could be an Array of either constructors or functions which match up to arguments, for example:
export default {
assertions: {
// This would throw an error, as it has no `predicate` key
'equal': {
'assert': (actual, expected) => actual === expected
},
// This would throw an error, as `predicate.length` does not match `assert.length`
'equal': {
'predicate': [Object],
'assert': (actual, expected) => actual === expected
},
'equal': {
// This predicate will match `actual instanceof Object` and `expected instanceof Object`
'predicate': [ Object, Object ]
'assert': (actual, expected) => actual === expected
},
'equal': {
// This predicate will match `React.isElement(actual)` and `React.isElement(expected)`
'predicate': [ React.isElement, React.isElement ]
'assert': (actual, expected) => myReactEqlAlgo(actual, expected)
}
}
}
(Having said all of that, I'd like to simplify the name predicate
to something like params
)
why do we need a predicate against the expected
param? I'm assuming that the developer who uses the plugin, at the call site, knows which of the overloaded namespace plugins she intends to use. So the only problem we need to solve here would be which plugin, out of many plugins with the same namespace, to choose for a given actual
value.
Am I making sense?
You're making sense, but I think there's maybe more utility in checking all params including expected
, as they could end up calling different code paths, for example:
export default {
assertions: {
'an': {
'predicate': [Object, String]
'assert': (actual, expected) => typeof actual === expected
},
'an': {
'predicate': [Object, Function]
'assert': (actual, expected) => actual instanceof expected
},
}
}
This could be an if
inside one function - but then we run into the same kind of problems we have right now.
fair enough.
@keithamus sorry for getting back to you that late, it's been a bit intense over here.
the proposal looks great! awesome work!
one thing I want to also focus on is the ability of modules like hippie
to utilize chai.js's assertions... but only what they need - eg. hippie might implement its own error class, have different error messages etc.
i have not put too much thought into this, so i know i'm describing it super vaguely. i hope things normalize soon so i can jump in and we start the road to the next major version.
ps. i'm super impressed with your contributions so far, hats down!
It caters to
expect
andshould
interfaces as first class, but does not create methods for theassert
interface.
Is this the reason why calling Chai.Assertion.overwriteMethod
doesn't work for assert(expected, actual, "Message")
?
Hi @Alhadis, thanks for your question.
I'm not sure I fully understand your doubt, but what @keithamus meant is that when using plugins to create assertions, those assertions created can only be applied when using the expect
and should
interfaces.
When using overwriteMethod
it does not work directly for any method on the assert
interface because the property you are overwriting is overwritten on the Assertion
prototype, which is then used under the hood by other assert
methods, as you can see here, for example. So, if you want to use overwriteMethod
and the assert
interface together you should see which methods from the Assertion
object your assert
method uses under the hood and then overwrite those methods.
Please let me know if I misunderstood your doubt or if you want to know anything else 😄
Ah right. No no, you understood my doubt correctly. :) Thanks for the speedy response! You guys rock.
I like this proposal too :) Making the assertion chain context aware will be nice.
@TiraO mentioned over in #1097
It's difficult to add chainable methods that work nicely with other chainable method plugins. Using
overwriteChainableMethod
to overwrite 'friendly' plugins feels like a workaround. It means that your plugin has to know about and depend on any plugins that it should work with.Right now the chaining framework can make behavior hard to understand or control as a plugin developer, which can result in unexpected behavior:
describe("when the end of the chain does not include an assertion", function () { it('can appear to be both true and false for the same chain', function () { expect({ x: 700, y: 100, z: 150 }).to.jsonEqual.roughly({ x: 700, y: 100, z: 150 }); expect({ x: 700, y: 100, z: 150 }).to.not.jsonEqual.roughly({ x: 700, y: 100, z: 150 }); }); }); describe("when the first method in the chain includes assertions", function () { it('can be accidentally and silently superceded by future assertions', function () { expect({ x: 700 }).to.not.jsonEqual.eql({ toJson: function () { return JSON.stringify({ x: 700 }) } }); }); });
I would like to see a clearer contract for chainable methods to communicate with each other. However, it would probably require a large overhaul of the api. Here are some features I might expect:
- allow chainable methods to wrap each other's comparators (instead of overwriting the entire assertion)
- combine assertion messages in a human-readable way
- raise an error when no assertions complete a chain
- chained methods do not have their own assertions
Obviously this is a complex problem and deserves more thought than I've put into it so far.
As we've been discussing a lot recently, we plan to make plugins a first class citizen for Chai v5 and make it easy for everyone to add plugins with assertions, modifiers and all that as easy as it is to add those things to the main library.
Due to house-cleaning purposes, I'll be closing this issue for now, but we'll definitely use this issue as reference and take into account what's been discussed here when implementing it.
I'm going to put this document here, as the start of a design discussion around how plugins could be written for a future version of chai. Similar discussions have been had before, over in Google Groups, and issues #117 and #457; but I wanted to start a new issue which can focus on the design of a new plugin architecture, and hopefully get some feedback.
Current Implementation
So, just to get everyone on track - right now, you can make a chai plugin by writing a little bit of code, like this:
Motivation
Right now we have a nicely expressive interface for creating plugins - but it has a few problems, which can be addressed by making working on a new interface. The problems it has are:
expect
andshould
interfaces as first class, but does not create methods for theassert
interface.addMethod
,addProperty
, andaddChainableMethod
. These in themselves aren't bad, but it does create a bit of an inconsistent interface, as it is an opt-in convention to use property assertions and chainable methods. Similarly, for users who only want to use method assertions, they have to use plugins like dirty-chai.expect(true).to.equal(true, 'True should be true!')
). Some plugins dont do this.eventually
to every method, which means programmatically rewriting every function. We should have an expressive syntax for this.not
. In fact,not
is so special that every assertion is required to support it. This should not be the case.chai.Assertion.addMethod
passes athis
value, and athis.assert
must be called within an assertion, to assert on anything. This could be simplified, and made less dependant on magic values likethis
.this.assert
has a list of ambiguous arguments, including boolean flags, and so it can be hard to decipher what is actually happening. If you don't passtrue
as the 6th argument - a diff wont show up for the assertion. This is a gotcha for even some of our best plugin authors.Requirements
Given the issues, we can set out a list of requirements that a new architecture should provide:
Must haves
addMethod
/addProperty
/addChainableMethod
functions.addMethod
should be able to be implemented much simpler - most small methods probably be distilled down to returning true/false. Making them "pure functions" (arguments are the only input, we determine what to do on the output, no side effects) would be an ideal.Nice to haves
expected {object} to {some expectation} but got {value}
- perhaps we could just have assertions declare the{some expectation}
part. We can generate error messages based on the actual keyword chain used for the assertion (e.g.expect(1).to.equal(2)
can beError: expected 1 to equal 2
).Draft concept
To start really thinking about a design - here is a draft concept I think could work when defining new assertions (this is an iteration on a previous draft in #117
Please note: