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, getter function always fail with expecting a property #1090

Closed
sukrosono opened this issue Nov 18, 2017 · 8 comments
Closed

Assert, getter function always fail with expecting a property #1090

sukrosono opened this issue Nov 18, 2017 · 8 comments

Comments

@sukrosono
Copy link
Contributor

sukrosono commented Nov 18, 2017

For the reproduction shake only assert.decreasesBy. See reproduction, line 16 pass; 18, 19 both fail.

AssertionError: 1: expected [Function: fn] to have property 'x'

But i suspecting:

  • assert.decreasesButNotBy
  • assert.increasesBy
  • assert.increasesButNotBy

When use getter function, let me know if i am wrong.
Discovered when made #1085 to aim #1084.

@sukrosono
Copy link
Contributor Author

It not feel right when expecting function to have property x. It do make sense when expecting object to have property x.

Any though @meeber , somehow expecting hope from you. Hopefully i mention on right time.

@keithamus
Copy link
Member

Hey @brutalcrozt thanks for the issue.

You're right, it is an awkward error message. We could perhaps look at improving this by adding an error message if you mix the arguments around. I'd be happy to see a PR of this nature!

@sukrosono
Copy link
Contributor Author

Assign me Cap, working on it.

I have a question, the 3rd param was written optional but when we use the getterFn i feel like need to have the 3rd param.
Then here still need 4th param delta. I found it awkward when we use assert(fn, getterFn, null, 1) for example. The 3rd param property as optional also found on several function, any advice?

@keithamus
Copy link
Member

It looks like you just need to throw when getterFn returns a number, while property and delta are supplied, right?

sukrosono added a commit to sukrosono/chai that referenced this issue Nov 21, 2017
@lucasfcosta
Copy link
Member

Hey folks, I'm not sure about the cases that are valid to throw here.

What if we have a property in a function that we want to check instead of running it?

This is a valid use-case and this is IMO what people would expect these assertions to do.

For example:

function myBook() {};
myBook.value = 10

function lowerValue() {
  myBook.value -= 1
}

assert.decreasesBy(lowerValue, myBook, 'value', 1);

This should not throw right?

However, whenever a property name is not provided I see no problem in running the second argument if it is a function.

@sukrosono
Copy link
Contributor Author

sukrosono commented Nov 21, 2017

Yet i am not trying throw anything.

This should not throw right?

It will fail with AssertionError: 1: expected undefined to be a number. I just inline it on test

I just questioning what's documented, i don't have any appropriate step to take.
nit: delta documented number, looks like string still pass. I just noticed, no offense. But when we found better options it will be great i guest.

Now i am confused, should discuss on issue or related PR then. I will follow any response for now.
I mention on PR comment, awaiting review and help regarding refactor tmpMsg

sukrosono added a commit to sukrosono/chai that referenced this issue Nov 23, 2017
This reverts commit 12a70f4.
sukrosono added a commit to sukrosono/chai that referenced this issue Nov 23, 2017
This reverts commit 12a70f4.
@sukrosono
Copy link
Contributor Author

sukrosono commented Dec 16, 2017

Recently i found that this is invalid, but still have things to discuss.

Hopefully i pick the word correctly. I refer the bdd style function signature, which is look good to me.

bdd decrease

bdd decrease


Highlight: (I mean by convention) Order matter, and required parameter on the left. Optional parameters on the right and are grouped.

Now let's look on tdd / assert interface:

decreases

decreases

also found on doesNotDecrease, plus the negation increases (2 things). Some parameter not in the group of optional parameter.


We read from top to bottom. The function signature (the first line) really matter, that is the first thing we read. Personally when i already get the point from first line, i stop reading an continue to write the code.




I see deeper violation

decreasesBy

decreasesby
also found on doesNotDecreaseBy, decreasesButNotBy. I am not check the increases yet, but it seems worth doing.

Some required parameter are placed between optional parameter.


Sure bdd and tdd (assert) is entirely different concept, and we use different approach. We take bdd with chainable method, but i am not sure with assert interface. My point just to make improvement. I will be happy to hear feedback or even pointing my misunderstanding.

@lucasfcosta
Copy link
Member

Since me and @keithamus are cleaning up issues I’ll close this because it will go into the roadmap.

As I demonstrated before, asserting on properties of functions is a valid use case.

I also mentioned that we can actually see if users did pass a property name or not by checking the number of arguments. If users pass 3 arguments then it means the third one is a value, not a property name (after all decreasesBy implies you will have to pass an argument which tells the delta). So this is what we’re going to do.

@chaijs chaijs deleted a comment from lucasfcosta Jun 9, 2018
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

3 participants