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

Feature request: .change/.increases etc with a function for value #544

Closed
elado opened this issue Oct 27, 2015 · 12 comments
Closed

Feature request: .change/.increases etc with a function for value #544

elado opened this issue Oct 27, 2015 · 12 comments

Comments

@elado
Copy link

elado commented Oct 27, 2015

// chai-change plugin doesn't seem to work anymore (chaijs/chai-change#4)

Seems like assertChanges in assertions.js expects object, prop. It limits the usage of this kind of assertion, as I can't do:

assert.increases(
  someOperation,
  () => getSomething().length
)

It's good in cases I use immutable data, and getSomething returns different object completely.

RSpec's change matcher has this feature, it can get either an object and a method name or a function for more arbitrary operations.

https://www.relishapp.com/rspec/rspec-expectations/docs/built-in-matchers/change-matcher

@elado elado closed this as completed Oct 27, 2015
@elado elado reopened this Oct 27, 2015
@keithamus
Copy link
Member

Hey @elado thanks for the issue.

If you'd like to make a PR for this feature I'd happily review it with a view to merging 😄. You'll need to modify the assertChanges assertion, as well as assertIncreases, and assertDecreases. Documentation is above each method (here's the assertchanges docs) - that will need updating with examples. Finally, extra tests will be needed to cover this, in all three interfaces: assert, expect, and should.

I look forwarding to seeing what you come up with 😀

@lucasfcosta
Copy link
Member

Hi @keithamus, I'm ready to tackle this one, can I or is anyone else already working on this?
If the answer is yes, should I raise it under the 4.x.x branch?

@keithamus
Copy link
Member

Hey @lucasfcosta - no one else has come forward to implement it, so it'd be awesome if you do! master should be fine to PR this against, as - if done carefully - it should be a backwards compatible change.

Also, I think it's important that @timruffles and @matthijsgroen are involved in this process (if they want to be) - as this overlaps a lot of the functionality from their plugins (aside: there's a larger issue here about chai plugins being subsumed as chai evolves - which I might open a new issue soon see #605 for more).

@lucasfcosta
Copy link
Member

Ah, okay, I totally understand, that issue explains the whole situation pretty well (and I think chai is in the right track here).

I will take a look at the code today and as soon as I've got a "draft" I will post it here so that @timruffles and @matthijsgroen can take a look, do further improvements and share their opinions on it.

Thanks for your help!

@matthijsgroen
Copy link
Member

Looks good so far. What my experience is that I used the from, to and by methods a lot as well, and use a lot of promises in my front-end code. Will this also be supported in this PR?

Or else the value of merging the 2 plugins in the core is a bit lost on me.

@keithamus
Copy link
Member

@matthijsgroen Promises is something I'd like chai to support, but in the right way. For example, in my experience, async/await pretty much eliminates the need for Promise support in chai. (Ninjaedit:) My point is we need to have a way to deal with promises with one wholesale pattern, that works for all assertions. Chai-as-promised demonstrates that possibility.

I'm slightly weary of adding such generic assertions as to and by - #339 is probably the right place to bikeshed those though. (ninjaedit:) I definitely think we should do something - and it should be expressive, but adding an assertion as generic as by just for changes does not seem right to me - I'm happy to be convinced otherwise.

I know all of that seems not very positive, but I really do want to accommodate those use cases, but in a way that makes sense to the whole of chai.

@matthijsgroen
Copy link
Member

I agree, but from the discussion in #339, the readability is more valuable than having some generic words around. What I understood when making these plugins is that these words are only allowed within certain chains, so this would protect people from making weird sentences no?

And I am happy to see an example of how this change thing would work with a promise. To be honest I wrote the plugins a while ago, still use them daily but I do not update the entire test stack daily to make use of all new plugins and constructs.

I'm not sceptical, I would love to have proper support in the core for these behaviours, but it should be there in a manner that would eliminate the need for change plugins instead of adding a 3rd option.

@lucasfcosta
Copy link
Member

Well, I've already got change, increase and decrease to accept a function as value, but I've done nothing regarding the by assertion. Should this be added to #607 too?

@keithamus
Copy link
Member

@lucasfcosta let's tackle it in a separate PR 😄

@lucasfcosta
Copy link
Member

Ah ok, I think the my PR has everything we need then
And thanks for the help! The guidelines you guys post on the issues make it very easy and pleasant for the contributors to work. 😉

@lucasfcosta
Copy link
Member

@keithamus

This one is done!
I think we can close this if @matthijsgroen and @timruffles agree

@timruffles
Copy link
Member

LGTM

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