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

Added tests and expectations for NaN. Ex: `expect(4).not.to.be.NaN;` #480

Merged
merged 1 commit into from Jul 16, 2015

Conversation

Projects
None yet
3 participants
@bradcypert
Contributor

bradcypert commented Jul 16, 2015

Added support for NaN as per @keithamus comment on issue #477

I agree. I think the to.be.NaN; structure is what makes the most sense for this issue.

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Jul 16, 2015

Member

Great work @bradcypert! Couple of notes:

  • Please don't compile chai.js, this is done per release. Could you please rebase and exclude the chai.js changes?
  • Could you please also add assert.isNaN and assert.isNotNaN - these are also important functions to be included.
  • Documentation is missing from this. Please refer to other addProperty calls, to see how we document chai properties.
  • Please also add tests for .should.be.NaN and so on, and tests for assert.is[Not]NaN - eventually I'll be adding coverage checking back into chai, and it'd be nice to make sure things are all properly covered 😜

Fix the above issues, and I'll be overjoyed to hit the big green merge button 😄

Member

keithamus commented Jul 16, 2015

Great work @bradcypert! Couple of notes:

  • Please don't compile chai.js, this is done per release. Could you please rebase and exclude the chai.js changes?
  • Could you please also add assert.isNaN and assert.isNotNaN - these are also important functions to be included.
  • Documentation is missing from this. Please refer to other addProperty calls, to see how we document chai properties.
  • Please also add tests for .should.be.NaN and so on, and tests for assert.is[Not]NaN - eventually I'll be adding coverage checking back into chai, and it'd be nice to make sure things are all properly covered 😜

Fix the above issues, and I'll be overjoyed to hit the big green merge button 😄

@bradcypert

This comment has been minimized.

Show comment
Hide comment
@bradcypert

bradcypert Jul 16, 2015

Contributor

I've done everything excluding the rebase, however, my assert tests are failing against saucelabs, but running locally.

Any chance you'd be interested in taking a look?

Contributor

bradcypert commented Jul 16, 2015

I've done everything excluding the rebase, however, my assert tests are failing against saucelabs, but running locally.

Any chance you'd be interested in taking a look?

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Jul 16, 2015

Member

Passing on travis, which is good enough for me 😉

Member

keithamus commented Jul 16, 2015

Passing on travis, which is good enough for me 😉

@bradcypert

This comment has been minimized.

Show comment
Hide comment
@bradcypert

bradcypert Jul 16, 2015

Contributor

Haha, fair enough. My git-fu is weak. How do I go about rebaseing to prevent my changes to chai.js?

Contributor

bradcypert commented Jul 16, 2015

Haha, fair enough. My git-fu is weak. How do I go about rebaseing to prevent my changes to chai.js?

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Jul 16, 2015

Member

I don't have much time right now - but could help out maybe tomorrow.

In the meantime you could read a tutorial, e.g. https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase. You just need to squash both commits and checkout the old copy of chai.js and then amend the commit with the older checked out copy.

Member

keithamus commented Jul 16, 2015

I don't have much time right now - but could help out maybe tomorrow.

In the meantime you could read a tutorial, e.g. https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase. You just need to squash both commits and checkout the old copy of chai.js and then amend the commit with the older checked out copy.

@bradcypert

This comment has been minimized.

Show comment
Hide comment
@bradcypert

bradcypert Jul 16, 2015

Contributor

Squashed everything and reverted my incidental change to Chai.js

Contributor

bradcypert commented Jul 16, 2015

Squashed everything and reverted my incidental change to Chai.js

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Jul 16, 2015

Member

Great work @bradcypert 😸

Member

keithamus commented Jul 16, 2015

Great work @bradcypert 😸

keithamus added a commit that referenced this pull request Jul 16, 2015

Merge pull request #480 from bradcypert/NaN-support
Added tests and expectations for NaN. Ex: `expect(4).not.to.be.NaN;`

@keithamus keithamus merged commit 5520c16 into chaijs:master Jul 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bradcypert bradcypert deleted the bradcypert:NaN-support branch Jul 16, 2015

@screendriver

This comment has been minimized.

Show comment
Hide comment
@screendriver

screendriver Nov 2, 2015

to.be.NaN; should be added to the docs 😉

screendriver commented Nov 2, 2015

to.be.NaN; should be added to the docs 😉

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Nov 2, 2015

Member

@screendriver it will be soon. The docs are pretty badly out of date but we're working on them 😄

Member

keithamus commented Nov 2, 2015

@screendriver it will be soon. The docs are pretty badly out of date but we're working on them 😄

@screendriver

This comment has been minimized.

Show comment
Hide comment
@screendriver

screendriver commented Nov 2, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment