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

Change by delta (New Assertion) #621

Merged
merged 4 commits into from
Feb 23, 2016
Merged

Conversation

lucasfcosta
Copy link
Member

As discussed on #339 this is the implementation of the by assertion.

I've tagged this as WIP because I want to confirm with you guys (especially @keithamus, since he is the one involved issue #339) how this is going to get implemented on the assert interface.

I thought about two possible ways to do it, here they are:

  1. Create six new methods, one for each of the change related methods of this interface.

    Doing this seems redundant and cumbersome, but this seems to follow the pattern of the rest of the interface.

    If we did this, we would have these new methods: changesBy, doesNotChangeBy, increasesBy, doesNotIncreaseBy, decreasesBy and doesNotDecreaseBy.

    I also think this is the most adequate approach.

  2. Return the created assertion and allow the user to chain the .by() assertion.
    The interface's methods would be something like:

    assert.changes = function (fn, obj, prop, msg) {
      if (arguments.length === 3 && typeof obj === 'function') {
        msg = prop;
        prop = null;
      }
    
      return new Assertion(fn, msg).to.change(obj, prop);
    }
    
    // This allows the user to write (for example):
    assert.changes(aFunction, anObject, 'aProperty').by(6);

    Personally, I don't like this because it adds a new kind of behavior to this interface. It seems to be cleaner but actually it causes a mess because it does not follow the other method's patterns

You can also feel free to suggest other improvements and point mistakes, I will always be open to others' opinions.

@keithamus
Copy link
Member

I think Option 1 sounds like the best idea 😄.

// This gets flagged because of the .by(delta) assertion
flag(this, 'msgObj', msgObj);
flag(this, 'initial', initial);
flag(this, 'final', final);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sold on the names of these flags. They're very likely only going to be used by the increase/decrease/change set - having then more specific to that would be good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed 👍
I'm gonna do it

@lucasfcosta
Copy link
Member Author

Thanks for your feedback! It was very valuable (as always) 😄

@lucasfcosta
Copy link
Member Author

Hi @keithamus, sorry for the delay on this. Well, I've got a doubt here about the behavior of .by involving .not.

When someone uses, for example, anyFunction.should.not.change(anObject, 'aKey').by(5), should it raise an exception only if the change had a delta value of 5? Currently if anyFunction changed the value of anObject by 10, for example, it would throw an Exception because it has changed and this sentence had should.not.change in it, I think this may not be the desired behavior.

If so, how can we detect there is a by assertion after change before even running the assertion?
I've been thinking about a way to do this but I couldn't find one.

(TL;DR; version: using should.not.change().by(delta) should throw an error only if the change had the specified delta?)

@keithamus
Copy link
Member

Presumably change() would end up throwing an error before by() got a chance to execute?

@lucasfcosta
Copy link
Member Author

Yes, exactly, currently I'm making change, increase and decrease call the by assertion, which will run the tests.

change, increase and decrease will only create flags that will be used by the by assertion.

I'll make sure to explain everything I did when adding it to this PR.

@lucasfcosta
Copy link
Member Author

Okay, so, after days analyzing this I came to a dead end, so let me explain the problem we have here and the possible solution:

Q: What is the problem?
A: When running assertions like: expect(fn).to.not.change(object, 'key').by(2) the negate flag gets applied both to change and by, so if fn changes 'key' (even if it's not by 2) it will fail before it even gets to the by.

Q: Why does this happen?
A: We currently run the change, increase and decrease assertions as soon as they get called. When we type expect(fn).to.not.change(object, 'key') the change assertion runs immediately, therefore when we run expect(fn).to.not.change(object, 'key').by(2) we should be saying "fn should not change 'key' by 2, every other change (or no change at all) is accepted" but we're actually saying "fn should not change 'key'" and then saying "it should not change 'key' by 2".
Got it?

Q: And how can we solve this?
A: The best idea I've had until now is to change the syntax to:

expect(fn).to.change(obj, 'key').but.not.by(2)

Using it this way the not getter would turn on the negate flag only for the by assertion.
Adding a 'but' word just for chain purposes also seems like a good idea.
This seems like the best approach because it really treats by as a separate assertion and keeps change, increase and decrease as clean as possible.

Also, on the assert interface I would rename doesNotChangeBy, for example, to changesButNotBy because it seems more semantically correct.

Please, @keithamus and others, feel free to give feedback/ideas.
If you accept this syntax I'm ready to commit it on this branch (and don't worry, I'll rebase to keep the commit log as clean as possible).

Thanks for your time

PS.: If you want more time to talk about this we can also chat through skype, hangouts or anything else.

@keithamus
Copy link
Member

@lucasfcosta thanks for the detailed description of the issue, however I'm not sure it is that much of a problem. If expect(fn).to.not.change(object, 'key') fails, then I would expect expect(fn).to.not.change(object, 'key').by(2) to also fail. Chai's chaining comes with an implicit agreement that the first failing assertion will fail the whole assertion.

You mention expect(fn).to.change(obj, 'key').but.not.by(2) as a workaround, which I think is a good workaround and the kind of syntax we should be aiming for. This should work out of the box right?

@lucasfcosta
Copy link
Member Author

@keithamus, Yes, it works right out of the box, because not gets the negate flag turned on only for the by assertion.

I just need to add but as a "chaining" only word and change the assertions on the assert interface.

I will make sure to make this PR as clean as possible so you can take a look at the final version of these changes.

Thanks for your feedback! I'll probably commit the code for this until tomorrow.

@lucasfcosta lucasfcosta force-pushed the change-by-delta branch 3 times, most recently from 63701b3 to da0f50c Compare February 22, 2016 20:15
@lucasfcosta
Copy link
Member Author

Well, it's done.
I also did some minor changes to variable names and property checks related to change methods.
Please tell me if I've forgotten something. 😄

@keithamus
Copy link
Member

Great work @lucasfcosta. Happy to merge this in 😄

keithamus added a commit that referenced this pull request Feb 23, 2016
[WIP] Change by delta (New Assertion)
@keithamus keithamus merged commit a97b22f into chaijs:master Feb 23, 2016
@lucasfcosta lucasfcosta changed the title [WIP] Change by delta (New Assertion) Change by delta (New Assertion) Feb 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants