More thorough bigint relational comparison tests #1251

thejoshwolfe · 2017-09-29T14:42:13Z

Notably this introduces coverage for:

bigint against Infinity and NaN, which are special cases in the new algorithm.
Number.MAX_VALUE vs bigint values that are a little above and a little below it by a factor of 16.
bigint against bigint, including values greater than Number.MAX_SAFE_INTEGER that only differ by 1n.

Regarding Number.MAX_VALUE, my reading of the spec is that we should not test differing from that value by only 1, since Number.MAX_VALUE is an approximate value. However, a factor of 16 above and below seems far enough way that any reasonable approximation should recognize the difference.

I'm trying a new approach to writing tests, where instead of using runtime abstraction to get thorough and consistent coverage, I'm using a python script to generate this code. I didn't include the generator script in this PR, because it seemed like a one-off thing that wouldn't be generally useful. If there is interest in including this generator in test262, here's what I've got so far, and I already know the style would need cleaning up to fit test262's style (e.g. indentation width).

The generator approach made it very easy to separate tests into 16 separate files. I could separate these tests into even more files pretty easily if that'd be desired. For example, I could put Infinity and NaN into separate test files instead of the one file called -non-finite.

littledan · 2017-09-29T18:10:22Z

test/language/expressions/greater-than-or-equal/bigint-and-bigint.js

+  4. Else,
+    a. Let nx be ? ToNumeric(px). Because px and py are primitive values evaluation order is not important.
+    b. Let ny be ? ToNumeric(py).
+    c. If Type(nx) is Type(ny), return ? Type(nx)::lessThan(nx, ny).


You're hitting this case, right? If so, maybe the below spec text doesn't need to be quoted. Also, you could quote BigInt::lessThan, which is always hit here.

Good catch. The frontmatter needs to be specialized for the different files even more than this. I'll make that update.

littledan · 2017-09-29T18:10:54Z

test/language/expressions/greater-than-or-equal/bigint-and-bigint.js

+assert.sameValue(0x1fffffffffffff01n >= 0x1fffffffffffff02n, false);
+assert.sameValue(0x1fffffffffffff02n >= 0x1fffffffffffff01n, true);
+assert.sameValue(-0x1fffffffffffff01n >= -0x1fffffffffffff02n, true);
+assert.sameValue(-0x1fffffffffffff02n >= -0x1fffffffffffff01n, false);


Maybe test some more values, e.g., a combination of long and short BigInt values.

littledan · 2017-09-29T18:23:24Z

test/language/expressions/greater-than-or-equal/bigint-and-number-extremes.js

+assert.sameValue(1n >= -Number.MAX_VALUE, true);
+assert.sameValue(-Number.MAX_VALUE >= 1n, false);
+assert.sameValue(
+  0xfffffffffffff80000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000n >= Number.MAX_VALUE,


Would it make sense to have a test for the edge cases, namely the BigInt which represents one less than Number.MAX_VALUE, the BigInt which is Number.MAX_VALUE, and one greater than it?

I addressed this in the OP of this PR, but it might be good to throw a comment in the file as well. What do you think?

Sorry, I missed the explanation there. I disagree with your reading of the spec--I think Number.MAX_VALUE is well-defined.

Alright, I'll tighten up the test.

leobalter · 2017-09-30T16:09:06Z

instead of using runtime abstraction to get thorough and consistent coverage, I'm using a python script to generate this code

Are you saying you can't achieve a thorough and consistent coverage this way?

thejoshwolfe · 2017-10-02T18:19:31Z

Are you saying you can't achieve a thorough and consistent coverage this way?

Sorry. I'm saying that I am achieving the same level of runtime coverage. Instead of runtime logic, I'm using analogous logic in the code generator.

An example of this coverage is that the case of comparing 1n against 0 is specified once in the generator, and 8 cases are output in the code: 4 different operators, and 2 different ways to order the operands.

rwaldron · 2017-10-02T18:26:42Z

I'm using a python script to generate this code. I didn't include the generator script in this PR, because it seemed like a one-off thing that wouldn't be generally useful.

Why not use the existing test generator tool?

rwaldron · 2017-10-02T18:28:21Z

Please review the CI failures. The frontmatter is invalid.

Linting 16 files.
Linting complete. 4 errors found.
test/language/expressions/greater-than-or-equal/bigint-and-bigint.js: FRONTMATTER - No valid YAML-formatted frontmatter
test/language/expressions/greater-than/bigint-and-bigint.js: FRONTMATTER - No valid YAML-formatted frontmatter
test/language/expressions/less-than-or-equal/bigint-and-bigint.js: FRONTMATTER - No valid YAML-formatted frontmatter
test/language/expressions/less-than/bigint-and-bigint.js: FRONTMATTER - No valid YAML-formatted frontmatter

leobalter · 2017-10-02T18:35:17Z

test/language/expressions/greater-than-or-equal/bigint-and-bigint.js

+    c. If Type(nx) is Type(ny), return ? Type(nx)::lessThan(nx, ny).
+
+sec-numeric-types-bigint-lessThan
+BigInt::lessThan (x, y)


missing spaces, this is causing the yaml parsing to fail.

thejoshwolfe · 2017-10-02T18:38:37Z

Please review the CI failures. The frontmatter is invalid.

Oops. Should have known that was invalid yaml.

thejoshwolfe · 2017-10-02T18:42:25Z

Why not use the existing test generator tool?

If you're talking about tools/generator/generator.py, then I don't think it's the right fit for this usecase. I don't think the template and case design can generate what I'm doing with switching the order of the operands and inverting the condition for the non-or-equal operators. I can look into trying to use the existing generator, but writing something quick from scratch seemed much easier.

leobalter

From this point, we can take care of nitpicks in follow up PRs.

LGTM.

more thorough bigint relational comparison tests

18c0904

littledan reviewed Sep 29, 2017

View reviewed changes

address review feedback. also compare against -0

61091da

leobalter reviewed Oct 2, 2017

View reviewed changes

fix yaml formatting

222f258

leobalter approved these changes Oct 2, 2017

View reviewed changes

leobalter merged commit aca48e4 into tc39:master Oct 2, 2017

thejoshwolfe deleted the relational branch October 3, 2017 10:31

thejoshwolfe mentioned this pull request Oct 4, 2017

Equality comparison tests for bigint #1257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More thorough bigint relational comparison tests #1251

More thorough bigint relational comparison tests #1251

thejoshwolfe commented Sep 29, 2017

littledan Sep 29, 2017

thejoshwolfe Oct 2, 2017

littledan Sep 29, 2017

littledan Sep 29, 2017 •

edited

Loading

thejoshwolfe Sep 29, 2017

littledan Sep 30, 2017

thejoshwolfe Oct 2, 2017

leobalter commented Sep 30, 2017

thejoshwolfe commented Oct 2, 2017

rwaldron commented Oct 2, 2017

rwaldron commented Oct 2, 2017

leobalter Oct 2, 2017

thejoshwolfe commented Oct 2, 2017

thejoshwolfe commented Oct 2, 2017

leobalter left a comment

More thorough bigint relational comparison tests #1251

More thorough bigint relational comparison tests #1251

Conversation

thejoshwolfe commented Sep 29, 2017

littledan Sep 29, 2017

Choose a reason for hiding this comment

thejoshwolfe Oct 2, 2017

Choose a reason for hiding this comment

littledan Sep 29, 2017

Choose a reason for hiding this comment

littledan Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

thejoshwolfe Sep 29, 2017

Choose a reason for hiding this comment

littledan Sep 30, 2017

Choose a reason for hiding this comment

thejoshwolfe Oct 2, 2017

Choose a reason for hiding this comment

leobalter commented Sep 30, 2017

thejoshwolfe commented Oct 2, 2017

rwaldron commented Oct 2, 2017

rwaldron commented Oct 2, 2017

leobalter Oct 2, 2017

Choose a reason for hiding this comment

thejoshwolfe commented Oct 2, 2017

thejoshwolfe commented Oct 2, 2017

leobalter left a comment

Choose a reason for hiding this comment

littledan Sep 29, 2017 •

edited

Loading