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.

Sign up for GitHub

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

BigInt ToBoolean type coercion test #1241

Closed
wants to merge 3 commits into from
Closed

BigInt ToBoolean type coercion test #1241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47e35f5
don't test non-trivial number toString()
Sep 25, 2017
5405d37
no sense in testing if BigInt is defined when 0n syntax is used elsew…
Sep 25, 2017
46fba43
add ToBoolean support to typeCoercion.js
Sep 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 45 additions & 11 deletions harness/typeCoercion.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,46 @@ function testCoercibleToIntegerFromInteger(nominalInteger, test) {
}
}

function testCoercibleToBooleanFalse(test) {
test(undefined);
test(null);
test(false);
test(0);
test(-0);
test(NaN);
test(0n);
test("");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that I agree with the benefit of the harness function here (which I recognize is similar to others that have been added for these coercion tests). It's not clear in the test file below what is actually being tested, without also seeking out this file and stepping through the layers that have been created. If the test file below contained the following, it would be both sufficient and more easily understood by those that would benefit from test262:

assert.sameValue(Boolean(undefined), false);
assert.sameValue(Boolean(null), false);
assert.sameValue(Boolean(false), false);
assert.sameValue(Boolean(0), false);
assert.sameValue(Boolean(-0), false);
assert.sameValue(Boolean(NaN), false);
assert.sameValue(Boolean(0n), false);
assert.sameValue(Boolean(""), false);

Copy link
Member

Choose a reason for hiding this comment

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

the code being more verbose helps communicating what it is actually doing, as mentioned. I'd avoid adding too many extras to the harness file so it doesn't become more complex.

Copy link
Member

Choose a reason for hiding this comment

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

to avoid ambiguity, I agree with @rwaldron


function testCoercibleToBooleanTrue(test) {
test(true);
test(1);
test(-1);
test(Infinity);
test(-Infinity);
test(1n);
test("a");
test("true");
test("false");
test("0");
test(Symbol("1"));
test({});
test([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my comment above, this would be sufficient in a single file:

assert.sameValue(Boolean(true), true);
assert.sameValue(Boolean(1), true);
assert.sameValue(Boolean(-1), true);
assert.sameValue(Boolean(Infinity), true);
assert.sameValue(Boolean(-Infinity), true);
assert.sameValue(Boolean(1n), true);
assert.sameValue(Boolean("a"), true);
assert.sameValue(Boolean("true"), true);
assert.sameValue(Boolean("false"), true);
assert.sameValue(Boolean("0"), true);
assert.sameValue(Boolean(Symbol("1")), true);
assert.sameValue(Boolean({}), true);
assert.sameValue(Boolean([]), true);


// ToBoolean does not call ToPrimitive.
// In all of these cases, it's just an object, which is truthy.
testPrimitiveWrappers(false, "number", test);
testPrimitiveWrappers(true, "number", test);
testPrimitiveWrappers(false, "string", test);
testPrimitiveWrappers(true, "string", test);

function notReallyAnError(error, value) {
test(value);
}
testNotCoercibleToPrimitive("number", notReallyAnError);
testNotCoercibleToPrimitive("string", notReallyAnError);
}

function testPrimitiveWrappers(primitiveValue, hint, test) {
if (primitiveValue != null) {
// null and undefined result in {} rather than a proper wrapper,
Expand Down Expand Up @@ -231,10 +271,8 @@ function testNotCoercibleToNumber(test) {
// ToNumber: Symbol -> TypeError
testPrimitiveValue(Symbol("1"));

if (typeof BigInt !== "undefined") {
// ToNumber: BigInt -> TypeError
testPrimitiveValue(BigInt(0));
}
// ToNumber: BigInt -> TypeError
testPrimitiveValue(0n);
Copy link
Member

Choose a reason for hiding this comment

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

are we just throwing this away and assuming this file will only be used with BigInt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to decide one way or another. In some places in this harness file, BigInt literals are used unconditionally. This check here was pointless, since the file would not parse without BigInt support already. Additionally, the BigInt feature is required for this harness file according harness/features.yml.

This was also a concern when my String.prototype.indexOf type coercion test required BigInt support back in #1200 and #1221. I'm open to discussing more versatility in this regard.


// ToPrimitive
testNotCoercibleToPrimitive("number", test);
Expand Down Expand Up @@ -291,15 +329,9 @@ function testCoercibleToString(test) {
testPrimitiveValue(-0, "0");
testPrimitiveValue(Infinity, "Infinity");
testPrimitiveValue(-Infinity, "-Infinity");
testPrimitiveValue(123.456, "123.456");
testPrimitiveValue(-123.456, "-123.456");
testPrimitiveValue("", "");
testPrimitiveValue("foo", "foo");

if (typeof BigInt !== "undefined") {
// BigInt -> TypeError
testPrimitiveValue(BigInt(0), "0");
}
testPrimitiveValue(0n, "0");

// toString of a few objects
test([], "");
Expand Down Expand Up @@ -406,4 +438,6 @@ function testNotCoercibleToBigInt(test) {
testStringValue("0o8");
testStringValue("0xg");
testStringValue("1n");

testNotCoercibleToPrimitive("number", test);
}
24 changes: 24 additions & 0 deletions test/built-ins/Boolean/toboolean.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright (C) 2017 Josh Wolfe. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
esid: sec-boolean-constructor-boolean-value
description: ToBoolean type coercion tests via Boolean()
info: >
Boolean ( value )

1. Let b be ToBoolean(value).
2. If NewTarget is undefined, return b.

features: [BigInt, Symbol, Symbol.toPrimitive]
includes: [typeCoercion.js]
---*/

testCoercibleToBooleanFalse(function(falsy) {
assert.sameValue(Boolean(falsy), false);
});

testCoercibleToBooleanTrue(function(truthy) {
assert.sameValue(Boolean(truthy), true);
});
Copy link
Member

Choose a reason for hiding this comment

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

This should also be placed in 2 different files, at least. We are not trying to condense everything in the same test file. This is also not helpful if anything is failing.


// `Boolean(value)` (without `new`) never throws.
Copy link
Member

Choose a reason for hiding this comment

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

this line is not necessary