Tests for BitInt.asIntN #1191

thejoshwolfe · 2017-08-21T23:19:41Z

I intend to make more BigInt tests. This is what I have so far. I wanted to submit this PR to get feedback on organization/style/coverage/etc before duplicating and modifying these for BigInt.asUintN.

leobalter

some comments but most of them are only for minor fixes.

leobalter · 2017-08-22T14:22:56Z

test/built-ins/BigInt/asIntN/bigint-tobigint.js

+  BigInt.asIntN ( bits, bigint )
+
+  2. Let bigint ? ToBigInt(bigint).
+features: [Symbol]


this should add the arrow-function feature as well, or at least use regular functions instead.

leobalter · 2017-08-22T14:23:08Z

test/built-ins/BigInt/asIntN/bits-toindex.js

+  BigInt.asIntN ( bits, bigint )
+
+  1. Let bits be ? ToIndex(bits).
+features: [Symbol]


this should add the arrow-function feature as well, or at least use regular functions instead.

leobalter · 2017-08-22T14:25:06Z

test/built-ins/BigInt/asIntN/bigint-tobigint.js

+assert.throws(SyntaxError, () => BigInt.asIntN(0, "foo"));
+assert.sameValue(BigInt.asIntN(3, "10"), 2n);
+assert.sameValue(BigInt.asIntN(4, "12345678901234567890003"), 3n);
+assert.throws(TypeError, () => BigInt.asIntN(0, Symbol("1")));


the mixed sameValue and throws calls makes it so hard to read. I know this is esthetic, but it would help a lot if they are sorted.

I thought of that too. I organized it here in a depth-first traversal order of the spec for all this. It was easier for me to make sure I was getting some decent coverage with this organization, but I can easily sort it to make it clearer to read.

I might be nitpicking on this, so just do as you prefer. It's ok in both ways.

I might be nitpicking on this

I don't think you are, but we can always clean it up later.

leobalter · 2017-08-22T14:30:23Z

test/built-ins/BigInt/asIntN/bigint-tobigint.js

+
+function MyError() {}
+
+assert.sameValue(BigInt.asIntN(3, Object(10n)), 2n);


this is basically the same as testing with 10n, there's no coercion here, AFAICT.

There should be other tests - not for this PR - to test calling Object with a bigint.

Object(10n) creates a BigInt object, wrapping the primitive.

in this case, we should use new BigInt, explicit.

new BigInt throws, whereas Object(10n) creates a wrapper.

oh I see. Yes, Object(10n) seems like the only way to create an object with an internal [[BigIntData]].

@leo yes, similar to:

Object("string") instanceof String

Which will have:

[[PrimitiveValue]]: "string"

yes, I was nitpicking, requesting something like an explicit new String call instead of using Object.

Anyway, that cannot happen with the BigInt ctor.

leobalter · 2017-08-22T14:33:21Z

test/built-ins/BigInt/asIntN/bits-toindex.js

+assert.throws(TypeError, () => BigInt.asIntN(Symbol(0), 0n));
+assert.throws(TypeError, () => BigInt.asIntN(1n, 0n));
+assert.sameValue(BigInt.asIntN(Object(3), 10n), 2n);
+assert.sameValue(BigInt.asIntN({valueOf:()=>3}, 10n), 2n);


I don't think checking Object(3) is necessary as we already have this custom object here. Removing the previous line (with the Object call) should be fine, otherwise, it could be explicit using new Number.

Does it hurt to have tests like this? In some places in JS, Number wrappers are treated differently from other objects.

I don't really understand the preference for new Number of Object.

new Number is explicit. If we want to be precise at the point we compare an ordinary object with a valueOf method vs another object with a primitive value, this all goes back to the fact we need tests for Symbol.toPrimitive calls or .toString calls after valueOf.

I can add tests with Symbol.toPrimitive methods. I originally shied away from writing thorough tests for ToPrimitive for such a distant method as BigInt.asIntN, but if that's what we want, I can do that.

I figured at some point implementations are going to be using the same ToPrimitive implementation that they got test coverage for in another context. It seems like more work to violate the details of ToPrimitive for only BigInt.asIntN than to just use a common ToPrimitive implementation. So it seems not very important to test every corner case of every reachable abstract operation from every user-facing API, but I can see the appeal of that approach as well.

Should I throw in some Symbol.toPrimitive tests? If so, I think I agree that they and some of these other ToPrimitive tests should be sectioned out into a separate test file.

I agree, it's good to have extensive coercion tests. You're right.

I don't think we need to check in tests for all combinations of possible coercion tests x operations which coerce to BigInt. However, this is the first place where we're testing coercions exhaustively, so it's not a bad place to put them.

So are you saying that BigInt.asIntN is going to be the call that exhaustively tests the type coercion for ToBigInt? And then BigInt.asUintN would not need as thorough tests? Is that what we're suggesting?

other methods will still use similar tests, but after this part is done, anything next should be easier.

I just noticed that ? GetMethod(input, @@toPrimitive) can throw a TypeError if the [Symbol.toPrimitive] is not callable. I should probably add a test for that too.

I'm getting nervous about writing such thorough tests under the name "BigInt" for an aspect of the spec that has nothing to do with the BigInt proposal. This kind of thing makes #1197 appealing.

in a user level, this might not seem relevant, but the purpose of this project is to confirm the observable steps in the spec, in the order they are defined. This is crucial to mitigate inconsistency from browsers and even detect issues like those in the web reality matters.

In the moment we say it's relevant for the spec to say a TypeError should be thrown if @@toPrimitive is not callable, a test for that is important, and that's a part of the BigInt api being covered here.

Although, it's not necessary to write a complete coverage in a single Pull Request, it can be fixed through follow up Pull Requests and I'm happy to volunteer writing those.

leobalter · 2017-08-22T14:35:50Z

test/built-ins/BigInt/asIntN/bigint-tobigint.js

+
+assert.sameValue(BigInt.asIntN(3, Object(10n)), 2n);
+assert.sameValue(BigInt.asIntN(3, {valueOf:()=>10n}), 2n);
+assert.throws(MyError, () => BigInt.asIntN(0, {valueOf(){throw new MyError();}}));


This is also not observing calls to @@toPrimitive or calling the toString method. I believe these are worth to be in a new separate file, splitting into one for objects and other for direct primitives coercion, at least.

I'm not sure what you're getting at. Do you mean that the test doesn't sufficiently assert that it's being reached? This is converting to a numeric type, so I believe it would call valueOf.

We observe it calling valueOf only, we don't have any observations for Symbol.toPrimitive or .toString calls when valueOf is not available

I thought you were opposed to too many redundant combinations of tests, as you've been removing them from the repository. This test is trying to get at some of what's involved in type conversions, but doesn't do all possible tests. I don't think the distinctions you're raising are extremely likely to cause bugs.

I don't think the distinctions you're raising are extremely likely to cause bugs.

That's a common phrase before tests. :)

I thought you were opposed to too many redundant combinations of tests, as you've been removing them from the repository. This test is trying to get at some of what's involved in type conversions, but doesn't do all possible tests.

I don't think these would be redundant, but they would check other easy to observe units (getting and calling other methods). With that, we could also check the order of them being called, as valueOf throwing a custom error after a valid @@toPrimitive return, e.g.

leobalter · 2017-08-22T14:36:17Z

test/built-ins/BigInt/asIntN/bits-toindex.js

+assert.throws(TypeError, () => BigInt.asIntN(1n, 0n));
+assert.sameValue(BigInt.asIntN(Object(3), 10n), 2n);
+assert.sameValue(BigInt.asIntN({valueOf:()=>3}, 10n), 2n);
+assert.throws(MyError, () => BigInt.asIntN({valueOf(){throw new MyError();}}, 0n));


Same as mentioned above:

This is also not observing calls to @@toPrimitive or calling the toString method. I believe these are worth to be in a new separate file, splitting into one for objects and other for direct primitives coercion, at least.

leobalter · 2017-08-22T14:38:11Z

test/built-ins/BigInt/asIntN/length.js

+
+verifyNotEnumerable(BigInt.asIntN, 'length');
+verifyNotWritable(BigInt.asIntN, 'length');
+verifyConfigurable(BigInt.asIntN, 'length');


we're trying to get rid of these separate verify helpers. verifyProperty should be fine:

verifyProperty(BigInt.asIntN, 'length', { value: 2, enumerable: false, writable: false, configurable: true });

The same applies for other test files in this PR

leobalter · 2017-08-22T14:39:59Z

test/built-ins/BigInt/asIntN/arithmetic.js

+// Copyright (C) 2017 Josh Wolfe. All rights reserved.
+// This code is governed by the BSD license found in the LICENSE file.
+/*---
+esid: pending


we already have an esid: sec-integer.asintn

https://tc39.github.io/proposal-bigint/#sec-integer.asintn

What's the current policy on esid--do you add it for things that are at Stage 3/4, or only once it's integrated into the main spec?

esid should be pending when an id is not available. In this case, we can have a prediction in the proposal. esid: pending should be the last resource, as it's becoming common to see tests left this way without any fix by the original contributors. The sooner we have it, better. Otherwise, this will cause an extra maintenance cost.

Sounds good to me.

So the section won't be renamed from integer to bigint?

I guess yes, so sec-bigint.asintn. If it's still cloudy, let's keep esid: pending. I just need to include it into my future tasks as soon as it is defined.

leobalter · 2017-08-22T14:41:37Z

test/built-ins/BigInt/asIntN/arithmetic.js

+  BigInt.asIntN ( bits, bigint )
+
+  3. Let mod be a BigInt representing bigint modulo 2**bits.
+  4. If mod ≥ 2**bits - 1, return mod - 2**bits; otherwise, return mod.


all the test files should add a features: [BigInt, BigInt.asIntN].

These features should be added to the features list as well: https://github.com/tc39/test262/blob/master/features.txt

I'm not sure if BigInt.asIntN is necessary to add here, maybe BigInt only.

cc @rwaldron

+1. I'd say BigInt is enough of a feature tag; I don't think we need feature tags for each little method of the feature.

I agree, we should follow only with BigInt for now. This can be easily fixed in the future if anyone provides another feedback.

The features.txt mentions Symbol as a "standard language feature" that's "largely maintained for historical reasons". How important is it to declare my dependency on that feature?

And for the arrow-function issue below, is there any value in me avoiding arrow functions to avoid depending on arrow functions? Is there any concern for allowing a partially-conformant implementation to test their support for BigInt without also needing support for arrow functions? If not, I can easily just throw arrow-function into the features declaration.

We are trying to flag every feature that date from ES6+. We missed a bunch for some ES6, but we still have a reasonable amount of Symbol uses.

I'm +1 to BigInt only. BigInt.asIntN is part of the whole BigInt

…y helper.

leobalter

This looks good. Any further improvements can be done in a follow up PR. e.g. the esid.

test cases for BitInt.asIntN

6705462

littledan approved these changes Aug 22, 2017

View reviewed changes

leobalter suggested changes Aug 22, 2017

View reviewed changes

leobalter added the coverage label Aug 22, 2017

Josh Wolfe added 2 commits August 22, 2017 14:41

add BigInt and arrow-function feature declarations. use verifyPropert…

fd76367

…y helper.

toString in addition to valueOf

3b776b7

thejoshwolfe mentioned this pull request Aug 24, 2017

Reusable harness utilities to test parameter type coercion #1197

Closed

Symbol.toPrimitive in addition to valueOf and toString

554f711

leobalter approved these changes Sep 6, 2017

View reviewed changes

leobalter merged commit 443b15e into tc39:master Sep 6, 2017

thejoshwolfe deleted the bigint branch September 7, 2017 14:35


		function MyError() {}

		assert.sameValue(BigInt.asIntN(3, Object(10n)), 2n);

Tests for BitInt.asIntN #1191

Tests for BitInt.asIntN #1191

Conversation

thejoshwolfe commented Aug 21, 2017

leobalter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejoshwolfe Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter left a comment

Choose a reason for hiding this comment

leobalter Aug 22, 2017 •

edited

Loading

thejoshwolfe Aug 23, 2017 •

edited

Loading

littledan Aug 22, 2017 •

edited

Loading