add Float16Array to all generic TypedArray tests #3849
Conversation
|
The harness doesn't include the bigint arrays, how are those tested? |
|
A separate harness: testBigIntTypedArray.js But they kind of have to be, since otherwise all consumers need to work with both Number and BigInt, which is pretty annoying; might as well just split them up. That's not true for Float16Array, since it's just another Number-containing TA kind. |
|
If the current split is by number type, then this approach seems fine. |
|
I like how this approach minimizes maintenance effort, but I've been wondering if the effect on consumers is acceptable. As far as I know (which admittedly isn't very far these days), Test262 doesn't make assertions conditionally anywhere else. I think there's value in a test suite that makes the same statement about conformance for everyone (e.g. when comparing results across implementations), so I don't want to give that up unintentionally. The only alternative comes to mind is to duplicate the tests. I'm not thrilled about that idea, either, but we might mitigate the worst of the maintenance hassle with a new rule for Test262's linter. |
|
I think in practice it still amounts to the same assertion - " If we were to duplicate the tests, I think the way to do it would be to migrate all of them to templates, and generate one test per typed array kind. (The current template system isn't ideally suited to this kind of thing, but it would suffice.) That way it would have sufficient granularity for an engine to fail only the Float16Array variants, without needing the logic itself to be duplicated in source. Buuuuuut that migration would be a lot of work; I wouldn't want to block adding these tests on it. (FWIW, @syg said the approach in this PR would work for V8.) |
|
fwiw I think it's very reasonable to land this now, with a future roadmap item to change these tests to be generated per-typed-array. |
|
Yes, I'd really like to see this landed as-is, unless we hear from a consumer that it's a big problem for them. As a consumer (in several projects) this works fine for me. Sounded like it was fine for V8 too. |
test/built-ins/TypedArrayConstructors/ctors/object-arg/returns.js
Outdated
Show resolved
Hide resolved
...ns/TypedArrayConstructors/internals/DefineOwnProperty/conversion-operation-consistent-nan.js
Outdated
Show resolved
Hide resolved
test/built-ins/TypedArray/prototype/fill/fill-values-conversion-operations-consistent-nan.js
Outdated
Show resolved
Hide resolved
|
@Ms2ger Done. I also updated one test which was defining but not using |
test/built-ins/TypedArray/prototype/set/typedarray-arg-set-values-same-buffer-other-type.js
Outdated
Show resolved
Hide resolved
|
|
@Ms2ger I'm not sure what you want me to do with that error message. Are you saying that I should fix the tests which currently lack that flag as part of this PR? |
|
It seems like the linter only runs on changed files, so your changes caused it to catch preexisting bugs 🤦 In order of preference, you could:
|
|
Done, but I'm not really looking forward to fixing the resulting merge conflicts... They're all trivial, it's just that it's a lot of them and I don't know of an easy way to make I'll get to it eventually though if no one else does. |
|
I rebased, thanks! |
|
I think this is ready then? |
| @@ -29,8 +30,17 @@ var typedArrayConstructors = [ | |||
| Uint8ClampedArray | |||
| ]; | |||
|
|
|||
| var floatArrayConstructors = typedArrayConstructors.slice(0, 2); | |||
| var intArrayConstructors = typedArrayConstructors.slice(2, 7); | |||
There was a problem hiding this comment.
intArrayConstructors did not contain Uint8ClampedArray but it is included now. This seems to be an intended change. Unfortunately, some Atomics tests (like built-ins/Atomics/add/good-views.js) still use intArrayConstructors. In other words, these tests are incorrect now. They should be rewritten to use testWithAtomicsFriendlyTypedArrayConstructors(), I guess.
There was a problem hiding this comment.
Thanks for the catch. Would you mind opening an issue for that, please?
There was a problem hiding this comment.
Oh, that was not an intended change, I just didn't realize that the .slice above stopped before the end of the list.
In fact the .slice was previously also omitting Uint8Array, which seems wrong. I've left it in, but opened #3984 to remove Uint8ClampedArray.
There was a problem hiding this comment.
I think that the original idea was to include Uint8ClampedArray in intArrayConstructors. The author just haven't realized that the second argument in typedArrayConstructors.slice(2, 7); is not the length but an (excluded) end index. That's why Uint8Array and Uint8ClampedArray were missing by accident. I find it correct to include Uint8ClampedArray in this array (it is a typed array with int elements). Some tests may skip Uint8ClampedArray unnecessarily when it is not included. That's why I though that the change was intentional and that just the problematic Atomics tests (that use intArrayConstructors without excluding Uint8ClampedArray) should be fixed/updated.
There was a problem hiding this comment.
@iamstolis Do you happen to have a list of such tests? It's a bit painful to track them down manually.
There was a problem hiding this comment.
The following tests started to fail (when run against graal-js engine). So, this is the correct list, hopefully.
Unexpectedly failed 'built-ins/Atomics/add/bad-range.js'
Unexpectedly failed 'built-ins/Atomics/add/good-views.js'
Unexpectedly failed 'built-ins/Atomics/and/bad-range.js'
Unexpectedly failed 'built-ins/Atomics/and/good-views.js'
Unexpectedly failed 'built-ins/Atomics/compareExchange/bad-range.js'
Unexpectedly failed 'built-ins/Atomics/compareExchange/good-views.js'
Unexpectedly failed 'built-ins/Atomics/exchange/bad-range.js'
Unexpectedly failed 'built-ins/Atomics/exchange/good-views.js'
Unexpectedly failed 'built-ins/Atomics/load/bad-range.js'
Unexpectedly failed 'built-ins/Atomics/load/good-views.js'
Unexpectedly failed 'built-ins/Atomics/or/bad-range.js'
Unexpectedly failed 'built-ins/Atomics/or/good-views.js'
Unexpectedly failed 'built-ins/Atomics/store/bad-range.js'
Unexpectedly failed 'built-ins/Atomics/store/good-views.js'
Unexpectedly failed 'built-ins/Atomics/sub/bad-range.js'
Unexpectedly failed 'built-ins/Atomics/sub/good-views.js'
Unexpectedly failed 'built-ins/Atomics/xor/bad-range.js'
Unexpectedly failed 'built-ins/Atomics/xor/good-views.js'
There was a problem hiding this comment.
@iamstolis Thanks, see #3984.
There was a problem hiding this comment.
Thanks, I run the tests from the current revision (62294b592e332bc16da6114e461fd7a8ed90daab) of #3984 against graal-js and I haven't seen any unexpected failures.
| * Array containing every non-bigint typed array constructor. | ||
| */ | ||
|
|
||
| var typedArrayConstructors = floatArrayConstructors.concat(intArrayConstructors); |
There was a problem hiding this comment.
This PR included Float16Array in typedArrayConstructors. Unfortunately, it failed to provide byteConversionValues.expected.Float16 (in harness/byteConversionValues.js). So, the tests that use testTypedArrayConversions(byteConversionValues, ...) fail when Float16Array is enabled. This seems to affect the following tests:
built-ins/TypedArray/prototype/fill/fill-values-conversion-operations.js
built-ins/TypedArray/prototype/map/return-new-typedarray-conversion-operation.js
built-ins/TypedArray/prototype/set/array-arg-src-tonumber-value-conversions.js
built-ins/TypedArray/prototype/set/typedarray-arg-set-values-diff-buffer-other-type-conversions-sab.js
built-ins/TypedArray/prototype/set/typedarray-arg-set-values-diff-buffer-other-type-conversions.js
built-ins/TypedArrayConstructors/ctors/object-arg/conversion-operation.js
built-ins/TypedArrayConstructors/internals/DefineOwnProperty/conversion-operation.js
built-ins/TypedArrayConstructors/internals/Set/conversion-operation.js
There was a problem hiding this comment.
@iamstolis Please open an issue - if no-one happens to have time to fix this right when they see it, it'll likely get lost out of their notifications.
There was a problem hiding this comment.
OK, I have created #3989
First step of #3828.
I've updated
harness/testTypedArray.jsso that it includes Float16Array iff it is available as a global, so as to not render all tests using it useless for any implementation which has not yet implemented Float16Array. I've also updated a bunch of tests which weren't using that helper to use it, so that they wouldn't require separate updates for Float16Array.In principle these should behave identically on existing implementations as prior to this PR.
I'll have a follow-up adding Float16Array-specific tests.