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

BigInt ToBoolean type coercion test #1241

Closed
wants to merge 3 commits into from

Conversation

thejoshwolfe
Copy link
Contributor

Add ToBoolean support to typeCoercion.js harness, and test 0n -> false, 1n -> true among other tests.

Josh Wolfe added 3 commits September 25, 2017 15:26
* Number.prototype.toString() has a lot of implementation-specified
  behavior, which means we shouldn't test something like
  (123.456).toString().
* Fix copypaste error
* ToBigInt error tests should also test ToPrimitive errors.
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

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);

assert.sameValue(Boolean(truthy), true);
});

// `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

test(NaN);
test(0n);
test("");
}
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


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.

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.

@thejoshwolfe
Copy link
Contributor Author

Really the main motivation for writing the toboolean.js test was to test that Boolean(0n) === false and Boolean(1n) === true. So maybe I should back up and just test that.

However, I'm a fan of the type coercion testing framework I've got going in typeCoercion.js; I think it's elegant, thorough, and clearly parallels the spec. Adding ToBoolean to the framework is a step in the direction I want it to go; eventually I'd like to see typeCoercion.js support all the To[Type] conversion functions.

I argue that what I've got in this PR is good organization. A test in the directory test/built-ins/Boolean/ directory makes a reference to ToBoolean utility functions, which parallels the spec's definition of the Boolean(value) function making a reference to the ToBoolean abstract operation. Then in harness/typeCoercion.js, there's a list of test cases for ToBoolean conversion that parallels the spec's table of types listed in the definition of the ToBoolean abstract operation.

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.

To answer this directly, the test is testing that Boolean(value) is calling the abstract operation ToBoolean(value). It's true that you can't tell what specific values are going into the Boolean() function, but that parallels the spec; it's not clear how the Boolean(value) function deals with specific types without stepping through the layers of abstraction in the spec. To me, this isn't a problem; this is good design both in the spec and in test262.

That's my argument for the elegance of this organization. You're also bringing up practical concerns, like having everything in one file means that if anything fails, then nothing else gets run. We can solve this problem at runtime without creating a combinatoric explosion of individual files. I'll make an independent PR with this idea.

@thejoshwolfe
Copy link
Contributor Author

I'll make an independent PR with this idea.

#1245

@littledan
Copy link
Member

This test only uses Boolean right now, but there are a number of contexts that call ToBoolean. For example, !!, or if statements. Would checking in tests that do that justify using the coercion framework here?

@leobalter
Copy link
Member

Would checking in tests that do that justify using the coercion framework here?

you're talking about a different thing, which doesn't apply here.

The current tests are checking the results for Boolean calls, which cannot be applied directly into the introduced harness functions.

What we have is:

  1. a general check for different types that coerce to false or true
  2. a harness helper and a file test - test/built-ins/Boolean/toboolean.js - that globs together all the units for type coercion and test Boolean at once, failing the general long-term purpose of Test262 to comprehend different units split in multiple files.
  3. a check for a BigInt literal added to the harness helper, preventing Boolean itself to guarded on implementations that are not ready to implement it. Test262 would be demanding or forcing implementations to work on this or just fail for everything.

Rather than elegant, these tests are against the design we've been aiming for more than 2 years. I'm not inclined to change the project structure design, neither if this starts with a single test file and not a more comprehensive structural reform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants