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

feat: BigInt Support #8382

Merged
merged 52 commits into from Sep 6, 2019
Merged

feat: BigInt Support #8382

merged 52 commits into from Sep 6, 2019

Conversation

JoshRosenstein
Copy link
Contributor

@JoshRosenstein JoshRosenstein commented Apr 26, 2019

Summary

Making some adjustments to support BigInt.
Partially Fixes #6829, add support in matchers. (not the babel syntax issue)

Currently this initial commit assumes we'd prefer to bake in BigInt support to the existing matchers rather than creating separate ones. This PR is WIP since there are remaining tweaks to be done but did not want to go down one route before opening it up for discussion.

Changelog

Features

Chore & Maintenance

Notes on initial commit:

[jest-get-type]

  • update getType & isPrimitive
  • Note: Changed the nested ifs to a switch statement, can revert to the old method if there are any concerns. Edit: reverted back ifelse due to failing test (but realized the issue was I didn't have 'object' as a fallback for unknown objects.)

[jest-matcher-utils]

  • Added ensureNumbersOrBigInt (Edit: add Bigint support to ensureNumbers )
    • Using this for matchers that accept numbers or BigInits,
  • **Need to discuss if we want shared or separate matchers
    • If shared, do we want to allow expect and received to be either or, or strictly one or the other (number vs bigint) (1n>=1 // true but 1n===1 // false)

[expect]

  • update jasmineUtils to check BigInit
  • update toBeGreaterThan toBeGreaterThanOrEqual toBeLessThan toBeLessThanOrEqual to allow biginit
  • All numeric checkers can have BigInit support baked in besides toBeCloseTo, therefore is why I had to keep ensureNumbers and add ensureNumbersOrBigInt (edit: toBeCloseTo will have bigint support )
  • ** Discuss if we we want separate matchers or not and if we want to allow mixed types to compare.

eslintrc.js

  • For ts overrides I disabled:
    • valid-typeof, no-undef , both are checked with ts, so dont have to inline eslint-ignore fore every biginit reference.
    • Another option for valid-typeof- bumping eslint-plugin-babel to add babel/valid-typeof

[babel-plugin-jest-hoist]

  • Add bigint in WHITELISTED_IDENTIFIERS

Test plan

For get-type, I added test in main test file but for the others I created temporary test files bigInt_temp.test, which I will merge into main test files in the future, Didn't want to have to keep re-updating snapshots on main test files since this is still WIP.
All tests are now within each repos main test file.

sample_test

Originally planned on just including biginit in ensurenumbers, but the only built in matcher that cant accept biginit would be toBeCloseTo
@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@codecov-io
Copy link

codecov-io commented Apr 26, 2019

Codecov Report

Merging #8382 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8382      +/-   ##
==========================================
+ Coverage   64.27%   64.29%   +0.02%     
==========================================
  Files         276      276              
  Lines       11700    11707       +7     
  Branches     2859     2864       +5     
==========================================
+ Hits         7520     7527       +7     
  Misses       3549     3549              
  Partials      631      631
Impacted Files Coverage Δ
packages/babel-preset-jest/index.js 100% <ø> (ø) ⬆️
packages/babel-plugin-jest-hoist/src/index.ts 0% <ø> (ø) ⬆️
packages/expect/src/spyMatchers.ts 81.79% <100%> (ø) ⬆️
packages/jest-get-type/src/index.ts 90.32% <100%> (+0.66%) ⬆️
packages/expect/src/matchers.ts 96.86% <100%> (+0.03%) ⬆️
packages/jest-matcher-utils/src/index.ts 82.58% <100%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8054b94...89b1538. Read the comment docs.

@jeysal
Copy link
Contributor

jeysal commented Apr 27, 2019

Hey @JoshRosenstein, thanks for taking care of this!

Discuss if we we want separate matchers or not and if we want to allow mixed types to compare.

I think using the existing matchers is the best way of doing this, let's see if someone wants to argue against it. Not yet sure if comparing numbers to bigints should be allowed, I think I tend towards no because you should know what kind of data type you're dealing with and this might help uncover inconsistencies in user code. It's also not really hard to convert to BigInt.

All numeric checkers can have BigInit support baked in besides toBeCloseTo, therefore is why I had to keep ensureNumbers and add ensureNumbersOrBigInt

I think toBeCloseTo should support it too! :)
It's not limited to decimal places, you can do expect(12).toBeCloseTo(10, -1) as well. This might be a rare use case, but I think it would be consistent to support BigInts there too, plus it allows us to have only ensureNumbers which deals with all kinds of numbers instead of two different variants. Edit: could even make it a matcher error when attempting to use BigInts with a precision > 0.

@pedrottimark
Copy link
Contributor

Comparing bigint to number in toBeGreaterThan toBeGreaterThanOrEqual toBeLessThan toBeLessThanOrEqual is reasonable expectation (pardon pun :) given specification for operators:

https://github.com/tc39/proposal-bigint#comparisons

Numbers and BigInts may be compared as usual. [examples of <, >, >=]

They may be mixed in arrays and sorted.

@pedrottimark
Copy link
Contributor

I think toBeCloseTo would be limited to same type for received and expected, both number or both bigint, because receivedDiff is computed by subtraction.

We would also need to replace Math.abs with unary minus, if the difference is negative.

Unrelated, I noticed this week an existing snapshot fails for toBeCloseTo in Node 12.0.0 :(

- Expected difference: not < 0.000005
+ Expected difference: not < 0.0000049999999999999996

Can confirm in Chrome 74 for Math.pow and ** operator but Firefox 67 still outputs 0.000005

@pedrottimark
Copy link
Contributor

pedrottimark commented Apr 28, 2019

After thinking more about toBeCloseTo and fixing a problem in #8389 and especially because meaningful precision for integers is a double negative, I suggest we leave it alone. If there is need for BigInt version, then a new matcher in jest-community seems like a reasonable solution.

EDIT: Sorry for the run-on sentence. My biggest concern is about meaning of precision:

  • for number: majority use case is digits to right of decimal point is positive precision argument
  • for number: minority use case for digits to left is tolerable to be negative precision argument
  • for bigint: no use case for digits to right of decimal point
  • for bigint: only use case for digits to left ought to be positive precision argument, but it does not seem reasonable to invert the meaning based on type of received and expected arguments, does it?

A smaller concern is need for function overloading in TypeScript to constrain both received and expected to have same type and imperative logic in function to enforce it in untyped environment.

@JoshRosenstein
Copy link
Contributor Author

Yeah, toBeCloseTo was question mark for me since the optional precision argument would then be a requirement for comparing integers, and then we would have to separate the logic between numbers & bigint since expected and received can be either or, if either is a bigint, would need to cast the number as a bigint before calculating receivedDiff, also as @pedrottimark mentioned, would need to replace any Math. methods that use expected/received, in this instance Math.abs . So that's what I was hesitant about and decided to go the route on creating ensureNumbersOrBigInt. However it would be less code to remove ensureNumbersOrBigInt, and add the seperate logic to handle bigint within toBeCloseTo...

@JoshRosenstein
Copy link
Contributor Author

JoshRosenstein commented Apr 29, 2019

For the case of including bigint within toBeClose, and removing ensureNumbersOrBigInt, ensureExpectedIsNumberOrBigInt, ensureActualIsNumberOrBigInt

the optional precision argument would then be a requirement for comparing integers

the workaround for this would be to set a default precision within the separated logic rather within the arguments and toBeCloseTo would look something like:

toBeCloseTo(
    this: MatcherState,
    received: number | bigint,
    expected: number | bigint,
    // Set to NaN in order to identify later if precision was passed or not
    precision: number = NaN,
  ) {
    const matcherName = 'toBeCloseTo';
    const secondArgument = arguments.length === 3 ? 'precision' : undefined;
    const options: MatcherHintOptions = {
      isNot: this.isNot,
      promise: this.promise,
      secondArgument,
    };
    ensureNumbers(received, expected, matcherName, options);

    let pass = false;
    let expectedDiff: number | bigint = 0;
    let receivedDiff: number | bigint = 0;

    //If both arent numbers, then one might be bigint
    if (typeof received === 'number' && typeof expected === 'number') {
      if (received === Infinity && expected === Infinity) {
        pass = true; // Infinity - Infinity is NaN
      } else if (received === -Infinity && expected === -Infinity) {
        pass = true; // -Infinity - -Infinity is NaN
      } else {
        // Set default precision for numbers
        precision = isNaN(precision) ? 2 : precision;
        expectedDiff = Math.pow(10, -precision) / 2;
        receivedDiff = Math.abs(expected - received);
        pass = receivedDiff < expectedDiff;
      }
    } else {
      // Set default precision for big integers
      precision = isNaN(precision) ? -2 : precision;
      if (precision >= 0) {
        //Should we throw some error for positive precision??
        // Or should we remove -precision from expectedDiff below
        throw new Error(
          matcherErrorMessage(
            matcherHint(matcherName, undefined, undefined, options),
            `${EXPECTED_COLOR(
              'precision',
            )} value must a negative number for BigInts`,
            printWithType('Precision', precision, printExpected),
          ),
        );
      }
      expectedDiff = BigInt(Math.pow(10, -precision) / 2);
      // cast as BigInt ensuring neither are a number to maintain precision
      receivedDiff = BigInt(expected) - BigInt(received);
      receivedDiff =
        receivedDiff >= 0 ? receivedDiff : receivedDiff * BigInt(-1);
      pass = receivedDiff < expectedDiff;
    }
...

Although its a rare case, I'm starting to lean towards this, rather than adding the extra checkers in jest-matcher-utils

D4nte added a commit to D4nte/DefinitelyTyped that referenced this pull request Apr 30, 2020
As per jestjs/jest#8382

TypeScript 3.2+ supports BigInt with esnext target.
TypeScript 3.8+ supports it with es2020 target.

dtslint is very particular on how the `typesVersions`
should be handled, hence the code duplication.
D4nte added a commit to D4nte/DefinitelyTyped that referenced this pull request May 4, 2020
As per jestjs/jest#8382

TypeScript 3.2+ supports BigInt with esnext target.
TypeScript 3.8+ supports it with es2020 target.

dtslint is very particular on how the `typesVersions`
should be handled, hence the code duplication.
D4nte added a commit to D4nte/DefinitelyTyped that referenced this pull request May 14, 2020
As per jestjs/jest#8382

TypeScript 3.2+ supports BigInt with esnext target.
TypeScript 3.8+ supports it with es2020 target.

dtslint is very particular on how the `typesVersions`
should be handled, hence the code duplication.
sheetalkamat pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request May 18, 2020
As per jestjs/jest#8382

TypeScript 3.2+ supports BigInt with esnext target.
TypeScript 3.8+ supports it with es2020 target.

dtslint is very particular on how the `typesVersions`
should be handled, hence the code duplication.
jjballano-qatium pushed a commit to jjballano-qatium/DefinitelyTyped that referenced this pull request Jun 16, 2020
As per jestjs/jest#8382

TypeScript 3.2+ supports BigInt with esnext target.
TypeScript 3.8+ supports it with es2020 target.

dtslint is very particular on how the `typesVersions`
should be handled, hence the code duplication.
ngbrown pushed a commit to ngbrown-forks/DefinitelyTyped that referenced this pull request Jul 11, 2020
As per jestjs/jest#8382

TypeScript 3.2+ supports BigInt with esnext target.
TypeScript 3.8+ supports it with es2020 target.

dtslint is very particular on how the `typesVersions`
should be handled, hence the code duplication.
@github-actions
Copy link

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 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BigInt support