Skip to content

Enhancement/expect-to-be-close-to-with-infinity#7444

Merged
rickhanlonii merged 10 commits into
jestjs:masterfrom
joao-conde:enhancement/toBeCloseTo-Infinity
Dec 2, 2018
Merged

Enhancement/expect-to-be-close-to-with-infinity#7444
rickhanlonii merged 10 commits into
jestjs:masterfrom
joao-conde:enhancement/toBeCloseTo-Infinity

Conversation

@joao-conde

@joao-conde joao-conde commented Nov 30, 2018

Copy link
Copy Markdown

Summary

For consistency it is desired that .toBeCloseTo can handle tests with Infinity.
As a tester, I want .toBeCloseTo to return true when testing Infinity with Infinity or -Infinity with -Infinity.
As a tester, I want .toBeCloseTo to return false when testing Infinity or -Infinity with any number.

Test plan

Run

yarn install

and after run the entire test suite with

yarn test

I also made some unit tests, testing for both Infinity and -Infinity with themselves aswell as both with real numbers.

My unit tests
my-tests

I ran the linter to make sure the code rules are enforced:

yarn-lint 1
yarn-lint 2

@joao-conde

joao-conde commented Nov 30, 2018 •
edited
Loading

Copy link
Copy Markdown
Author

Can I get some feedback on why CI is failing? I've run the linter and the tests and both passed locally

yarn-test

SimenB
SimenB approved these changes Dec 1, 2018
View reviewed changes
Comment thread CHANGELOG.md Outdated
@SimenB

SimenB commented Dec 1, 2018

Copy link
Copy Markdown
Member

Restarted CI for you. Looked intermittent

@joao-conde

joao-conde commented Dec 1, 2018

Copy link
Copy Markdown
Author

@SimenB CI still failling yet I do not understand the error message, could you elucidate me please? Thank you in advance

rickhanlonii
rickhanlonii reviewed Dec 1, 2018
View reviewed changes
Comment thread CHANGELOG.md Outdated
Comment thread packages/expect/src/matchers.js Outdated
@SimenB
Apply suggestions from code review
c03e22f 
Co-Authored-By: joao-conde <up201503256@fe.up.pt>
rickhanlonii
rickhanlonii approved these changes Dec 1, 2018
View reviewed changes

@rickhanlonii rickhanlonii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after you fix the changelog 👍

Comment thread packages/expect/src/__tests__/matchers.test.js
Comment thread packages/expect/src/matchers.js Outdated
@joao-conde

joao-conde commented Dec 1, 2018

Copy link
Copy Markdown
Author
  1. Applied the suggestion for the CHANGELOG item order
  2. Tried to make comments more explicit
  3. Ran linter to ensure code standards
  4. Ran the expect package tests again

rickhanlonii
rickhanlonii reviewed Dec 1, 2018
View reviewed changes
Comment thread CHANGELOG.md Outdated
rickhanlonii
rickhanlonii reviewed Dec 1, 2018
View reviewed changes
Comment thread packages/expect/src/matchers.js Outdated
@joao-conde

joao-conde commented Dec 1, 2018 •
edited
Loading

Copy link
Copy Markdown
Author

If there is anything else I can improve please let me know. What would it take for the it to be merged and close the issue #7405 ?

rickhanlonii
rickhanlonii approved these changes Dec 2, 2018
View reviewed changes

@rickhanlonii rickhanlonii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rickhanlonii rickhanlonii merged commit 624ac56 into jestjs:master Dec 2, 2018
@rickhanlonii

rickhanlonii commented Dec 2, 2018

Copy link
Copy Markdown
Member

Thanks @joao-conde and congrats on your first Jest PR!

@joao-conde

joao-conde commented Dec 2, 2018

Copy link
Copy Markdown
Author

@rickhanlonii the first of hopefully many, thank you!

@joao-conde joao-conde deleted the enhancement/toBeCloseTo-Infinity branch December 2, 2018 01:04
@SimenB SimenB mentioned this pull request Dec 2, 2018
Closed
thymikee added a commit to spion/jest that referenced this pull request Dec 18, 2018
@thymikee
Merge branch 'master' into pr/6871
0a4ae67 
* master: (24 commits)
  Add `jest.isolateModules` for scoped module initialization (jestjs#6701)
  Migrate to Babel 7 (jestjs#7016)
  docs: changed "Great Scott!" link (jestjs#7524)
  Use reduce instead of filter+map in dependency_resolver (jestjs#7522)
  Update Configuration.md (jestjs#7455)
  Support dashed args (jestjs#7497)
  Allow % based configuration of max workers (jestjs#7494)
  chore: Standardize filenames: jest-runner pkg (jestjs#7464)
  allow `bail` setting to control when to bail out of a failing test run (jestjs#7335)
  Add issue template labels (jestjs#7470)
  chore: standardize filenames in e2e/babel-plugin-jest-hoist (jestjs#7467)
  Add node worker-thread support to jest-worker (jestjs#7408)
  Add `testPathIgnorePatterns` to CLI documentation (jestjs#7440)
  pretty-format: Omit non-enumerable symbol properties (jestjs#7448)
  Add Jest Architecture overview to docs. (jestjs#7449)
  chore: run appveyor tests on node 10
  chore: fix failures e2e test for node 8 (jestjs#7446)
  chore: update docusaurus to v1.6.0 (jestjs#7445)
  Enhancement/expect-to-be-close-to-with-infinity (jestjs#7444)
  Update CHANGELOG formatting (jestjs#7429)
  ...
willsmythe pushed a commit to willsmythe/jest that referenced this pull request Dec 20, 2018
@willsmythe
Enhancement/expect-to-be-close-to-with-infinity (jestjs#7444)
88e48ab 
* toBeCloseTo works properly when comparing with Infinity

* CHANGELOG update

* Linter issue solved

* Apply suggestions from code review

Co-Authored-By: joao-conde <up201503256@fe.up.pt>

* Update matchers.js

* Update CHANGELOG

* Fixed bug, leftover of old comment

* Removed unecessary self-explanatory comments

* Updated CHANGELOG

* Snapshot updates
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@captain-yossarian
Enhancement/expect-to-be-close-to-with-infinity (jestjs#7444)
517081b 
* toBeCloseTo works properly when comparing with Infinity

* CHANGELOG update

* Linter issue solved

* Apply suggestions from code review

Co-Authored-By: joao-conde <up201503256@fe.up.pt>

* Update matchers.js

* Update CHANGELOG

* Fixed bug, leftover of old comment

* Removed unecessary self-explanatory comments

* Updated CHANGELOG

* Snapshot updates
@github-actions

github-actions Bot commented May 12, 2021

Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@SimenB SimenB SimenB approved these changes

@rickhanlonii rickhanlonii rickhanlonii approved these changes

Assignees

No one assigned

Labels

cla signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@joao-conde @SimenB @rickhanlonii @facebook-github-bot