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

Add lint friendly assertions #48

Closed
wants to merge 1 commit into from

Conversation

joshperry
Copy link

Starting in chai 1.10.0 there are callable assertion terminators available; see chaijs/chai#297. This change checks for this functionality and uses it if available.

These both will now be valid ways to assert for called, calledOnce, calledTwice, and calledThrice.

spy.should.have.been.calledOnce;
spy.should.have.been.calledOnce();

@domenic
Copy link
Collaborator

domenic commented Nov 5, 2014

I don't support this on principle.

@domenic domenic closed this Nov 5, 2014
@joshperry
Copy link
Author

Which principle would that be? The fact that code with no apparent side effects is deemed bad enough form that linters throw errors on it, or do you perhaps feel that test code isn't important enough for static analysis?

I'm happy to change the PR to be more in-line with your principles but I don't know how it could get more simple than a single line of code that adds a simple new feature from upstream with absolute backwards compatibility.

@domenic
Copy link
Collaborator

domenic commented Nov 5, 2014

It was discussed in the Chai issue. chaijs/chai#41

@joshperry
Copy link
Author

Yes, I was a part of that discussion, I'm also the one that wrote the implementation for chaijs/chai#297, which caused them to reopen chaijs/chai#41 and should be merged today.

I'm confused by why you are refusing your users an arguably valuable feature just because you don't like the form, especially when the existing form still works exactly as it did before with absolutely no changes.

Beside every other argument against getters with side-effects -- especially throwing exceptions --, and squelching lint errors in tests, the biggest reason I would like to remove these from my test code is because it prevents false-positives. You do realize that javascript will happily execute func.should.have.been.caledOnce at runtime with nary an error, causing the test to falsely pass. Stupid typos like these are not the kind of thing I want to spend time chasing around, especially when there are tools that will catch them for me and direct me to the precise file and line where the error was made.

If this simple change is something you're patently against merging, then I'll just direct people in that thread to my fork for when they find it through a google search like I did.

@bajtos
Copy link

bajtos commented Nov 5, 2014

The biggest reason I would like to remove these from my test code is because it prevents false-positives. You do realize that javascript will happily execute func.should.have.been.caledOnce at runtime with nary an error, causing the test to falsely pass. Stupid typos like these are not the kind of thing I want to spend time chasing around, especially when there are tools that will catch them for me and direct me to the precise file and line where the error was made.

+1000

@keithamus
Copy link
Member

@domenic I'd ask you to reconsider this - especially because we have another issue on Chai related to this (chaijs/chai#272) - which adds another feature: allowing the message to be overridden on these assertions, using should (e.g. foo.should.be.calledOnce('Needs to be called Once'))

If you'd like, I can do the work necessary to make this a PR. It should be a really small change, and it seems like there is a good amount of users wanting it.

@domenic
Copy link
Collaborator

domenic commented Nov 6, 2014

If/when accepting this pull request adds new functionality in that way, I'll reconsider. Not before.

@keithamus
Copy link
Member

Good to know @domenic, thanks for your work on sinon-chai 😄

@joshperry
Copy link
Author

Thanks guys for looking at this. I think both features will be a huge benefit to users, it's obviously a hot topic.

@domenic
Copy link
Collaborator

domenic commented Nov 10, 2014

I would appreciate it if nobody added +1s to this thread. I am leaving it un-locked because I want to let the OP amend it once the PR adds new functionality, but until then as I said about this will not be accepted.

@mockdeep
Copy link

It kind of seems like the message thing is tangential to this feature. Not sure I see why the two need to be part of the same pull request.

@domenic
Copy link
Collaborator

domenic commented Nov 10, 2014

Please read the whole thread before commenting. Without new functionality, I do not agree with this pull request.

Locking.

@chaijs chaijs locked and limited conversation to collaborators Nov 10, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants