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

Assert an increase/decrease by an amount (delta) #339

Closed
keithamus opened this issue Jan 2, 2015 · 10 comments
Closed

Assert an increase/decrease by an amount (delta) #339

keithamus opened this issue Jan 2, 2015 · 10 comments

Comments

@keithamus
Copy link
Member

As per #333 and #330, we should be able to assert on the delta of a .changed/.increased/.decreased.

So far (in #333) the proposed syntaxes are:

foo.should.change('bar').by(5);
foo.should.increase('bar').by(5);
foo.should.decrease('bar').by(5);

or

foo.should.change('bar', 5);
foo.should.increase('bar', 5);
foo.should.decrease('bar', 5);

/cc @oveddan @cmpolis @logicalparadox

@lexi-sh
Copy link

lexi-sh commented Jan 23, 2015

The former is closer to the rest of the syntax in chai. In my opinion we should always aim for tests to be human readable, and the 5 in the second set just seem like magic numbers. I see some discussion about how it is uncomfortably generic, but if you really think about other assertions I can't think of a way for the by keyword to be used elsewhere.

@timruffles
Copy link
Member

both @matthijsgroen's chai-changes and chai-change (disclaimer: mine) plugins support this, along with async assertions (promises in Matthijs' and callbacks in mine).

I guess the change of maintainer explains why the changes feature didn't land in 2013 in #218, I'm not bitter ;)

@keithamus
Copy link
Member Author

Hey @timruffles, sorry about the overlap - wasn't aware of either of these plugins at the time of merging the original PR. As you ant @matthijsgroen both have plugins which cater for this - do you have any input on this issue? Even if that input is "don't do it" 😄.

@timruffles
Copy link
Member

I really like the by feature! It's in both the linked plugins.

If I was to make a suggestion, it's that the current implementation in core doesn't support async use-cases, or functional style:

e.g the below - which is bread and butter node webdev - isn't possible in the current implementation:

it('creates a version', function(done) {

    assert.change(function(cb) {
        create({
            type: "plan",
            name: "v2",
        }, cb)
    }, function(cb) {
            Version.count({}, cb);
    }, { 
        callback: done, by: 1,
    });

});

@matthijsgroen
Copy link
Member

https://github.com/matthijsgroen/chai-changes#changebydelta

The changes library supports async, automatically if the return value of the when(function() { }) is a promise.

@lucasfcosta
Copy link
Member

+1 for the by feature.
I really think having by improves readability, which is one of the main things that makes Chai awesome.

Also, I can't think of a better way to implement it other than having an assertion just for it.
Even if we have it just because of changes, increases and decreases I still think this is a price worth paying.

@keithamus
Copy link
Member Author

Well, as I've said a few times before - I'm here as a steward, not a dictator. So if people want by then I can't argue.

PRs welcome 😄

@lucasfcosta
Copy link
Member

Hi, @keithamus, how are you?
Well, I think #607 is done and ready to be merged.
I will be pleased to work on this as soon as the change related changes are ready for a merge. Can I?

PS.: I hope I'm not bothering you, feel free to answer whenever you want 😃

@keithamus
Copy link
Member Author

Feel free to make a PR for this @lucasfcosta. You're not bothering me, its amazing to have such a good contributor eager to contribute more!

@lucasfcosta
Copy link
Member

I think we can close this one, @keithamus !
And thanks for the feedback you gave me on the PR #621

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants