Reusable harness utilities to test parameter type coercion

[thejoshwolfe]

As I'm working some tests through review here #1191 (comment) , the question came up of how thoroughly to test type coercion for API parameters. Specifically, the first parameter to BigInt.asIntN is passed into the abstract operation ? ToIndex(bits). To test that implementations are doing everything in ToIndex on that parameter, a pretty large number of test cases needs to be written. Not just testing the RangeErrors in ToIndex, but also the floor(abs(number)) in ToInteger, the TypeErrors in ToNumber, the user-definable hooks in ToPrimitive and ToOrdinaryPrimitive, etc. And then after writing all these tests, copy-paste them all for BigInt.toUintN, which also calls ? ToIndex(bits).

As an idealistic software engineer, I of course tried to think of how to reduce this duplication. Here's my first draft of an idea (that for sure needs a lot of cleanup before it would be acceptable): https://gist.github.com/thejoshwolfe/bb00255524eeca6396269e1221c96a4a

My first feelings about this kind of approach is that it's too complicated to be worth the maintenance effort.

On the plus side though, this makes the test harness parallel the spec a bit more closely. You can write test cases for the abstract operation ToInteger in one place (the generic harness utility function), and then when an API calls ToInteger, the API test can call the test for ToInteger. This means that when ToNumber adds a row that BigInt should throw a TypeError, it would be theoretically possible to add that case to one place in the tests and get coverage through many APIs.

In general, this approach should be possible for ToIndex, ToInteger, ToNumber, and many other type coercion abstract operations.

[bterlson]

I like any test helper that encodes a best-practice in a way that can easily be leveraged by test authors. But I wonder if this sort of thing would be better implemented as a generator that creates individual test files for each scenario as is the current practice? It does help with finding exactly where errors occurred.

[littledan]

If you can do this as a file that's placed in the harness/ directory, then this would be following the pattern of many other tests. We can discuss the details further in a code review.

[leobalter]

a helper or a generation cases would both be great. There's an implied complexity here where the operations will happen and how they affect the result.

The other problem is the clarity. A single test file failing is not the best usage. Any failure will prevent any of the remaining code to execute, split test files provides a better overview of what exactly is failing and perhaps implementation progress.

There are also observable points to consider, e.g. the TypedArray constructor. TypedArray ( buffer [ , byteOffset [ , length ] ] )

ToIndex is a common operation for this constructor, used with byteOffset and length in different ways. The problem here goes beyond as in some valid results for ToIndex the constructor might still return an abrupt completion. This is a fact for many other api parts.

I'm welcoming for a good solution to improve this, but so far, common usage is the best I've got.

Maybe as @littledan, having it as a harness/ file - or even with tests for it, would be easier to see and discuss.

[rwaldron]

Summoning @jugglinmike for additional input/feedback

[thejoshwolfe]

This is similar to what I'm proposing: https://github.com/tc39/test262/blob/d9f62e4ccf11b2248c2045fb980bf4255621bab0/harness/testAtomics.js

The other problem is the clarity. A single test file failing is not the best usage.

I'm not sure how pertinent clarity is in this case. I could be wrong, but I imagine most failures from the tests I'm proposing would be fairly obvious to diagnose. I'm imagining the only kinds of bugs that would cause these failures are something like: 1) You're not running the parameter through your generic ToIndex coercion function; maybe you used ToLength or ToInteger by mistake. 2) Your implementation of ToIndex has a bug, which should be observable through nearly every API that's supposed to use ToIndex.

However, I recognize that might be naive about just how complexly implementations can get seemingly simple things wrong.

[jugglinmike]

This seems like a nice helper to me.

I'm always wary of abstraction in tests, and the generation tool could be used
produce more literal test material. That said, I think it's a slippery slope to
use the tool to essentially explode a function call or two. The original
motivation for the tool was to promote coverage across syntactic forms. Doing
that programatically would require eval, which would produce some pretty ugly
tests, ones that relied on additional semantics. That's not a concern in these
cases where a traditional function will do.

@leobalter is right that it may obscure the full extent of implementation bugs,
but I think that's okay for this case. The generic result "ToInteger is faulty"
is probably enough for the general audience interested in conformance, and
implementers are likely going to have to dig in further no matter what
granularity we report here.

[thejoshwolfe]

I'll make a PR independent of BigInt that demonstrates this concept for String.prototype.indexOf. That way I can run the tests to make sure they work, and I believe there's missing coverage for passing a Symbol to indexOf anyway (for all 3 parameters).

[thejoshwolfe]

now included in #1221.

[littledan]

Seems like this facility was checked in, but then later, there's been some hesitation about using it, e.g., in #1256 which removed use of this facility in one case. Are we OK with typeCoercion.js, or should we be revisiting the decision to land it and go back to the earlier suggestion of test generation? I'm reviewing #1284 which adds to this file, in a way which seems to be good to me, but I don't want to make changes that are out of step with the rest of the maintainers.