Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added change, increase and decrease assertions with by chain (#330) #333

Merged
merged 8 commits into from Jan 2, 2015

Conversation

@cmpolis
Copy link
Contributor

@cmpolis cmpolis commented Dec 28, 2014

Added change, increase and decrease assertions with by chain:

buildUser = function() { ... };
removeUsers = function() { ... };
buildUser.should.change(users, 'length');
buildUser.should.change(users, 'length').by(1);
removeUsers.should.decrease(users, 'length');
...

I've used change a bunch in other testing setups and it allows for more readable and terse tests (instead of before=val ... val.should.eq(before+1)).

* var obj = { val: 10 };
* var fn = function() { obj.val = 10 };
* var noChangeFn = function() { return 'foo' + 'bar'; }
* expect(fn).to.change(obj, 'val');

This comment has been minimized.

@keithamus

keithamus Dec 28, 2014
Member

Doesn't look like fn actually changes the value of val, right?

This comment has been minimized.

@cmpolis

cmpolis Dec 29, 2014
Author Contributor

Ahhh, good catch.


var initial = object[prop];
fn();
var delta = object[prop] - initial;

This comment has been minimized.

@keithamus

keithamus Dec 28, 2014
Member

It might be nice if .change just tests for inequality, rather than be restricted to number types.

So I can do person.capitalizeName().should.change('name')

This comment has been minimized.

@cmpolis

cmpolis Dec 29, 2014
Author Contributor

Agreed - will put in tests + fix for this.


Assertion.addChainableMethod('change', assertChanges);
Assertion.addChainableMethod('changes', assertChanges);
Assertion.addChainableMethod('Change', assertChanges);

This comment has been minimized.

@keithamus

keithamus Dec 28, 2014
Member

I can't recall any other chainable methods that have capitals. I think this could probably be left out.

This comment has been minimized.

@cmpolis

cmpolis Dec 29, 2014
Author Contributor

Thanks! I wasn't too sure about this - I saw the capitals in some other places in this file and was trying to follow those patterns: Assertion.addProperty('Arguments', checkArguments); Assertion.addMethod('Throw', assertThrows);

This comment has been minimized.

@keithamus

keithamus Dec 29, 2014
Member

Ah, I forgot about those. They are just because arguments and throw are reserved property names in ES3, while Arguments and Throw aren't. However change is not a reserved word, and so should be fine to omit.


Assertion.addChainableMethod('increase', assertIncreases);
Assertion.addChainableMethod('increases', assertIncreases);
Assertion.addChainableMethod('Increase', assertIncreases);

This comment has been minimized.

@keithamus

keithamus Dec 28, 2014
Member

Same as above, aliases with capitals aren't really used elsewhere AFAIK


Assertion.addChainableMethod('decrease', assertDecreases);
Assertion.addChainableMethod('decreases', assertDecreases);
Assertion.addChainableMethod('Decrease', assertDecreases);

This comment has been minimized.

@keithamus

keithamus Dec 28, 2014
Member

And again

@keithamus
Copy link
Member

@keithamus keithamus commented Dec 28, 2014

Great PR @cmpolis! Thanks for adding docs and tests!

I've made a couple of comments on the code above, that I'd like to see addressed. I also have some general notes...

  • I can see the value in .change() and .increase() and .decrease(), and I'm mostly happy with the functionality (apart from the above comments).
  • I'm not entirely sold on the .by() assertion; it is a very generic keyword for such a specific set of functionality. It seems like it could be folded into the other assertions, for example:
foo.should.change('bar', 5)
foo.should.increase('bar', 5)
foo.should.decrease('bar', 5)
@cmpolis
Copy link
Contributor Author

@cmpolis cmpolis commented Dec 29, 2014

Sweet, thanks! I updated the pr w/ the notes above. As far as by goes - I'm fine either way, my preference is to have more natural language oriented tests and I think by aids in that. Also, it takes out some ambiguity on parameter order.

@keithamus
Copy link
Member

@keithamus keithamus commented Dec 29, 2014

@cmpolis thanks for addressing all of my above points. PR is looking really good to me.

As for the .by method, I can definitely see your points; .by does read better, but I worry about it for the reasons mentioned. To get this PR moving though, if anyone else has any opinions on this (especially @logicalparadox or @vesln, or the original reporter - @oveddan) then I'd like to read through them!

@logicalparadox
Copy link
Member

@logicalparadox logicalparadox commented Dec 29, 2014

I am going to go with an additional argument for the change value.

expect(fn).to.change(foo, 'bar', 5);
  • It is consistent with existing principles: expect(foo).to.have.property('bar', 'baz')
  • .by is uncomfortably generic (but could be implemented easily as a plugin).
@oveddan
Copy link

@oveddan oveddan commented Dec 29, 2014

I think by is better as it's more readable.

Also, using optional arguments is less flexible to enhancements and extensibility to the matcher down the line, as that argument can only be one thing.

by doesn't have to be generic; when change is used, it can change the subject of the assertion to be the changed value, just as property does:

expect(obj).to.have.property('foo')
  .that.is.a('string');

And then by could only be allowed to be chained to change matchers.

@oveddan
Copy link

@oveddan oveddan commented Dec 29, 2014

Another option is instead of overloading change, could have a changeBy matcher as well that would require the changed amount for an argument.

@keithamus
Copy link
Member

@keithamus keithamus commented Dec 30, 2014

You may be on to something - about .change modifying the assertion subject. Here are some thoughts about it:

  1. If .change modified the subject to be the property questioned (flag('obj', object[prop])), you could have more flexibility in your assertions - but would lose the ability to assert on delta without extra matchers. e.g.:

    bar = { val: 5 };
    function foo() { bar.val += 5; }
    foo.should.change(bar, 'val').and.be.a('number').and.be.above(5)
    //                           ^ here the new prop is `bar.val` (10)
  2. If .change modified the subject to be the delta (flag('obj', delta)) you could still see how much a value was changed by - but the resulting code would be a little more magical

    bar = { val: 5 };
    function foo() { bar.val += 5; }
    foo.should.change(bar, 'val').and.be.a('number').and.eql(5)
    //                           ^ here the new prop is `delta` (5)

My complete personal opinion: I don't like either of these. But prefer both to adding .by.

@cmpolis
Copy link
Contributor Author

@cmpolis cmpolis commented Dec 30, 2014

foo.should.change(bar, 'val').and.be.a('number').and.be.above(5);
foo.should.change(bar, 'val').and.be.a('number').and.eql(5);

This syntax would be ambiguous to me if I hadn't seen the internals: is it the change that should == 5 or bar.val that should == 5

Also, change becomes kind of useless in the first example since you know(or are making an assumption) about the initial state:

bar = { val: 5 };
function foo() { bar.val += 5; }
foo.should.change(bar, 'val').and.be.a('number').and.be.above(5)

might as well be:

bar = { val: 5 };
function foo() { bar.val += 5; }
foo();
bar.val.should.be.a('number').and.be.above(5)
@keithamus
Copy link
Member

@keithamus keithamus commented Dec 30, 2014

I agree with all of your points @cmpolis. Perhaps for now we agree to disagree about the .by functionality, and remove it from the PR, and bring it up in a new issue. I think everyone is on board with .change, .increase and .decrease, so we could work on getting just that set of functionality merged with this PR, and then go into further discussions on extending this.

@cmpolis
Copy link
Contributor Author

@cmpolis cmpolis commented Dec 30, 2014

Okay, sounds good! I'll stash .by and update the pr.

If everyone is cool with expect(fn).to.change(foo, 'bar', 5);, I can make that rewrite and put it in this pr.

cmpolis added a commit to cmpolis/chai that referenced this pull request Dec 30, 2014
@cmpolis cmpolis force-pushed the cmpolis:change-assertions branch from b1fceb8 to 581cf83 Dec 30, 2014
cmpolis added a commit to cmpolis/chai that referenced this pull request Dec 30, 2014
@cmpolis
Copy link
Contributor Author

@cmpolis cmpolis commented Dec 30, 2014

messed up on b1fceb8, should be an easy fix/merge, but lmk if I should rebase my branch

* var obj = { val: 10 };
* var fn = function() { obj.val = 15 };
* expect(fn).to.increase(obj, 'val');
* expect(fn).to.increase(obj, 'val').by(5);

This comment has been minimized.

@keithamus

keithamus Dec 31, 2014
Member

.by still exists in the documentation

* var obj = { val: 10 };
* var fn = function() { obj.val = 5 };
* expect(fn).to.decrease(obj, 'val');
* expect(fn).to.decrease(obj, 'val').by(5);

This comment has been minimized.

@keithamus

keithamus Dec 31, 2014
Member

and again (.by is documented but doesnt exist)


assert.increasesBy = function (fn, obj, prop, amount) {
new Assertion(fn).to.increase(obj, prop).by(amount);
}

This comment has been minimized.

@keithamus

keithamus Dec 31, 2014
Member

Should these be removed, because the .by method doesn't exist?


assert.decreasesBy = function (fn, obj, prop, amount) {
new Assertion(fn).to.decrease(obj, prop).by(amount);
}

This comment has been minimized.

@keithamus

keithamus Dec 31, 2014
Member

Same as above (probably needs removing as .by doesnt exist any more)

@keithamus
Copy link
Member

@keithamus keithamus commented Dec 31, 2014

Hey @cmpolis - I've made a couple of notes about lingering .by docs/methods which should be removed.

Also, I just noticed that in your commits you've removed chai.js. If you could rebase your commits to leave that file alone, that'd be swell 😄

As soon as that's done, I'll do another quick once over but it should be good to merge! At which point I'll also draw up a new issue discussing adding .by or similar.

@cmpolis cmpolis force-pushed the cmpolis:change-assertions branch from 07dfb46 to a669234 Jan 2, 2015
@cmpolis
Copy link
Contributor Author

@cmpolis cmpolis commented Jan 2, 2015

Sorry... accidentally committed before coffee 😄 Rebased and put in those changes you noted, thanks!

@keithamus
Copy link
Member

@keithamus keithamus commented Jan 2, 2015

LGTM 😄

keithamus added a commit that referenced this pull request Jan 2, 2015
Added `change`, `increase` and `decrease` assertions with `by` chain (#330)
@keithamus keithamus merged commit 5fc486b into chaijs:master Jan 2, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@keithamus keithamus mentioned this pull request Feb 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants