-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
feat: BigInt Support #8382
Conversation
Originally planned on just including biginit in ensurenumbers, but the only built in matcher that cant accept biginit would be toBeCloseTo
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! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Hey @JoshRosenstein, thanks for taking care of this!
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
I think |
Comparing bigint to number in https://github.com/tc39/proposal-bigint#comparisons
|
I think We would also need to replace Unrelated, I noticed this week an existing snapshot fails for - Expected difference: not < 0.000005
+ Expected difference: not < 0.0000049999999999999996 Can confirm in Chrome 74 for |
After thinking more about EDIT: Sorry for the run-on sentence. My biggest concern is about meaning of precision:
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. |
Yeah, |
For the case of including bigint within toBeClose, and removing
the workaround for this would be to set a default precision within the separated logic rather within the arguments and 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 |
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.
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.
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.
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.
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.
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.
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. |
Summary
Making some adjustments to support BigInt.
PartiallyFixes #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
[expect]
- Add BigInt Support totoBeGreaterThan
,toBeGreaterThanOrEqual
,toBeLessThan
toBeLessThanOrEqual
andtoBeCloseTo
(feat: BigInt Support #8382)[jest-get-type]
- Add BigInt Support. (feat: BigInt Support #8382)[jest-matcher-utils]
- Add BigInt Support toensureNumbers
ensureActualIsNumber
,ensureExpectedIsNumber
(feat: BigInt Support #8382)[babel-plugin-jest-hoist]
- Add BigInt toWHITELISTED_IDENTIFIERS
(feat: BigInt Support #8382)[babel-preset-jest]
- Add @babel/plugin-syntax-bigint(feat: BigInt Support #8382)Chore & Maintenance
[eslintrc]
- Disable valid-typeof, no-undef for ts overrides (feat: BigInt Support #8382)Notes on initial commit:
[jest-get-type]
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(Edit: add Bigint support to ensureNumbers )ensureNumbersOrBigInt
[expect]
updatejasmineUtils
to check BigInittoBeGreaterThan
toBeGreaterThanOrEqual
toBeLessThan
toBeLessThanOrEqual
to allow biginitAll numeric checkers can have BigInit support baked in besides(edit:toBeCloseTo
, therefore is why I had to keepensureNumbers
and addensureNumbersOrBigInt
toBeCloseTo
will have bigint support )eslintrc.js
[babel-plugin-jest-hoist]
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.