Change function for value

[lucasfcosta]

This is my first draft for #544.
(EDIT: I think there's nothing more to be done, I read the code again and it seems to match all the orientations on #544)

• I have added checks on the assert interface in order to maintain compatibility and allow different arguments
• assertChanges, assertIncreases and assertDecreases now accept only a function as the object argument instead of needing both object and prop
• Tests were added on all three interfaces

I tried to avoid code repetition, but all assertions have basically the same inner workings, so this is the cleanest way I could code them.

Is this looking like what you had expected @keithamus? If it is not feel free to point out the changes you'd like me to make.

@timruffles and @matthijsgroen are also welcome to take a look on this and share their opinions 😃 .

[keithamus]

Good work @lucasfcosta 👍

[mrgenixus]

Is there any discussion of supporting functions as in https://github.com/timruffles/chai-change, or would chai-change override if used?

[keithamus]

This PR adds function support. It seems like we're basically going to be folding in chai-change's featureset - not something with initially planned for though.

[mrgenixus]

I just read through the PR code more thoroughly, thanks for adding this.

[timruffles]

It'd be good to support functions as obviously in node we test stuff that's async. e.g assert.changes(User.count, () => User.insert({}), { by: 1 }).

[timruffles]

It's a shame the original chai-change code couldn't be used as it supported all of this, and had tests.

Totally put off contributing by: 1) being told to put something in a plugin, 2) then having chai override it and breaking compat with the plugin 3) and finally now, having a version without all the features added not using the original, working, testing, feature complete code that @matthijsgroen or I wrote :/

[keithamus]

@timruffles please read #605. Understand this was not an intentional thing that happened, and we'd definitely like to fold in the features from chai-change and chai-changes, in a way that is idiomatic to Chai core. If you'd like, we could discuss this properly over something like Skype or Google Hangouts?

[matthijsgroen]

I would like to contribute, but not sure what exactly you need from me :-) (all code is open source anyway ;-)) I haven't upgraded Chai in a while, so my tests are stable at the moment. If you need something from me, please let me know. I agree with the approach of having Chai support all basic structures of Javascript out of the box. And I hope asynchronous behaviour will be supported everywhere, but more than that opinion (which you guys already know) I am not able to add at the moment :-)

Thanks for adding me to the group!

[amireh]

I must agree with @timruffles here; even if the effort put by the authors of chai-change(s) was duplicated, it's pretty bad that Chai basically overwrites those plugins. Users are left with little option now...

It seems the APIs that Chai presents are pretty opinionated, consider this:

var x = 0;
assert.changes(function() { x += 1 }, function() { return x }, { by: 1 });

Which yields:

AssertionError: expected [Function] to have a property { by: 1 }

I don't understand the decision that the modifier must return an object - what's the point of defining the getter? I think in the above scenario the user would expect that the expectations ({ by: 1} here) are applied against the exact return value of the getter.

If I find the time and you guys are willing to accept a contribution, I'd like to make it so that Chai (as of 3.5.0) allows us to use chai-change and chai-changes (then there'll be no harm done to existing Chai API users and everyone'll be happy - aside of the fragmentation that happened here...)

Thoughts? (Thanks to everyone for the great work, needless to say :))

[lucasfcosta]

Hi @amireh, thanks for your feedback!

I'm afraid I didn't understand exactly what you think.

What is a "modifier" to you? Is it the function which changes a value?

Actually, you don't necessarily need to return an object on the modifier function, you just gotta pass to the assertion the property of which object you expect to change. As you can see here what we do is:

1. We get the value of the property passed by the user
2. We run the function which does (or not) the change
3. We compare the values before and after

I think Chai's current API already covers the same cases as the chai-change plugin (which is an excellent plugin btw), we just incorporated it's functionalities into Chai keeping our assertion style, which aims to be fluid and as similar to the english language as possible.

That example you just posted works for the change assertion, and not for the by assertion.
For the by assertion you could use changesBy instead of changes.
You can see the code for these assertions and take a look at the whole PR which introduced this change here.

Please let me know if I misunderstood what you wanted to say.
I would also like to thank you for offering your help, thanks buddy 😄

[amireh]

What is a "modifier" to you? Is it the function which changes a value?

Yes! Sorry about the confusion, I think chai-change calls it "affector" - we're talking about the same thing though.

1. We get the value of the property passed by the user

I believe this is the point of my misconception; to me when I used the API, my reasoning was for a function of the following signature:

assert.change(fnAffectValue, fnGetValue, descriptionOfExpectedChanges)

Was that fnGetValue will be called before and after the application of fnAffectValue and the yields of fnGetValue will be asserted against based on the expectations described by the 3rd parameter. Now, it seems that it's very common that you're asserting against object properties but that wasn't the case for me; I'm just asserting against a scalar that fnGetValue will take care of retrieving for Chai.

Maybe I could wrap the output in an object and then it would work, but that seems a bit hackish to me.

What I'm proposing (but this would be an API-breaking change) is: should a getter function be passed for a value (fnGetValue) then Chai stops making assumptions about where the value is; it expects the function to return a scalar (in our case, a number, since I wanted to assert a change by: 1).

Does that make any sense?

I think Chai's current API already covers the same cases as the chai-change plugin (which is an excellent plugin btw), we just incorporated it's functionalities into Chai keeping our assertion style, which aims to be fluid and as similar to the english language as possible.

Chai is brilliant, we use it everyday and in every project that is testable - I'd love to contribute if I can. The API itself is engineered towards the common use case which is totally understandable, but it would be nice to give us the low handles when needed, that's all.

That example you just posted works for the change assertion, and not for the by assertion.
For the by assertion you could use changesBy instead of changes.

I did not know about that assertion, thank you! I'll check out the PR for more hidden goodies. It's too bad that the docs here do not mention changesBy and I was formerly looking at chai-change's docs which provide a little different API.

[lucasfcosta]

@amireh thanks for taking your time to explain 😄

Well, the docs there aren't updated because they're generated according to the latest version released on NPM (which is 3.5.0 by now) and this change is going to be released on 4.0.0.

The change you proposed really seems useful. Having to assign a variable to an object in order to test it seems a bit hackish indeed. Having the possibility of using a getter function would be great IMO.

@meeber @keithamus What do you guys think about this?

If people agree we can push this forward. I can give you more guidelines to start your work as soon as we get to an agreement with other maintainers here. But basically we would need to check the passed arguments to check if we've got a function as the second argument and then use that getter function to retrieve initial and final values,

Thanks for wanting to contribute! Let's keep on rocking 💥

[meeber]

Sounds reasonable to me for change and changesBy to accept a function that returns a value as an alternative to an object / property combo.

When documenting this, I'd avoid use of the word getter in the solution, as an "object getter" seems to be a subset of the functionality being proposed. The full functionality being proposed is the ability to assert that any given function returns a different value when called before and after another function. I think we can refer to this function as the subjectFn, valueFn, targetFn, or the previously mentioned fnGetValue.

[amireh]

Personally, I find either subjectFn or getValueFn to be clear enough.

When is 4.0.0 planned for release?