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 .does as a no-op assertion, Fix #700 #701

Merged
merged 2 commits into from
May 7, 2016

Conversation

vieiralucas
Copy link
Member

Hello guys, I saw that #700 was marked as pull-request-wanted.
So here I am 😄

I also did not find tests addressing no-op assertions.
Maybe I could add tests for it.

What do you guys think about it?

@lucasfcosta
Copy link
Member

@vieiralucas is right at my side right now at college, I also watched him coding, so this LGTM.

I think it would be a great idea to have this kind of tests.
Putting them on the expect and should interfaces should be enough, since there's no way we can do this kind of chaining on the assert interface (and since it uses should behind the scenes).

@meeber
Copy link
Contributor

meeber commented May 6, 2016

Hey guys I was thinking about the same thing earlier. I think a really simple test for each of the language chains would definitely be good. And I agree it should appear in both the should and expect interface tests for thoroughness.

I didn't realize you two were acquaintances! This Lucas camaraderie is more dangerous than I feared... I need to find a second Meeber, and fast.

@vieiralucas
Copy link
Member Author

photo on 5-6-16 at 10 00 pm
Thinking about this PR

I'll do the tests, make sure to rebase this in order for the commits to make sense and keep the git logs clean.

@vieiralucas
Copy link
Member Author

I added the only test that I could think of, which just checks that the chain exists.
What else should be tested?

@meeber
Copy link
Contributor

meeber commented May 7, 2016

I'd say validate that each of them is chaining correctly, even though grammatically it won't always make sense:

    expect(3).to.equal(3);                                                      
    expect(3).and.equal(3);
    expect(3).of.equal(3);
    ...etc...

Also a nitpick: The indent-style in the test arrays is different than the assertions.js array.

@vieiralucas
Copy link
Member Author

I see, the problem is that the error will be

TypeError: Cannot read property 'equal' of undefined

That's why I used .not.undefined instead of .to.not.be.undefined

But that's what you get when you use chai to test chai right?
Others tests already have this behavior.
I was wondering, have you consider using node's Assert to test chai?

Anyways, I'll add tests that validade if they are chaining correctly.

@vieiralucas
Copy link
Member Author

Looking closer I realized that it will be testing that (3).equal(3) still works.
Now I understand

@vieiralucas vieiralucas force-pushed the does branch 2 times, most recently from d43287f to 16fb110 Compare May 7, 2016 03:53
@meeber
Copy link
Contributor

meeber commented May 7, 2016

LGTM!

@lucasfcosta
Copy link
Member

lucasfcosta commented May 7, 2016

Awesome, good job guys!
LGTM

@lucasfcosta lucasfcosta merged commit 8e781c4 into chaijs:master May 7, 2016
@vieiralucas vieiralucas deleted the does branch May 7, 2016 15:35
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.

3 participants