add more Float16Array cases, and tests for Math.f16round by bakkot · Pull Request #4017 · tc39/test262

bakkot · 2024-03-12T18:42:55Z

Followup to #3849. Fixes #3828. (~~Edit: actually I need the dataview methods also, which I'll try to get to soon. Doesn't invalidate these, it's just that more tests are needed.~~ Done.)

The structure of the TypedArray tests is such that you can't easily add tests for just one kind of typed array, so the values I'm adding need to be included in the "expected" lists for each of the typed array types. I automated that rather than doing all those by hand, but I did confirm the three major browser engines all agree (for the non-float16 cases; for the float16 ones I used conversion code I wrote and checked against other sources).

For the get/setFloat16 tests, I just copied the get/setFloat32 tests and modified as appropriate.

test/built-ins/Math/f16round/value-convertion.js

ptomato

I started reviewing this but didn't finish. Here's what I have so far.

ptomato · 2024-03-14T00:19:37Z

harness/byteConversionValues.js

-      0
+      0,   // 0
+      1,   // 2049
+      3,   // 2051
+      0,   // 0.00006103515625
+      0,   // 0.00006097555160522461
+      0,   // 5.960464477539063e-8
+      0,   // 2.9802322387695312e-8
+      0,   // 2.980232238769532e-8
+      0,   // 8.940696716308594e-8
+      0,   // 1.4901161193847656e-7
+      0,   // 1.490116119384766e-7
+      224, // 65504
+      240, // 65520
+      239, // 65519.99999999999
+      0,   // 0.000061005353927612305
+      0    // 0.0000610053539276123


I didn't check how this is specified, but presumably these are equal to truncate(value) modulo 256, with the appropriate sign modifications for signed int8?

Yup (though as I say in the OP the values for the non-float16 cases are derived by doing the conversions on existing engines on the assumption they're correct, rather than by hand).

ptomato · 2024-03-14T00:42:25Z

test/built-ins/DataView/prototype/getFloat16/length.js

+assert.sameValue(DataView.prototype.getFloat16.length, 1);
+
+verifyNotEnumerable(DataView.prototype.getFloat16, "length");
+verifyNotWritable(DataView.prototype.getFloat16, "length");
+verifyConfigurable(DataView.prototype.getFloat16, "length");


We're trying to prefer verifyProperty over the individual functions in new tests (command applies to other tests of property descriptors as well)

Suggested change

assert.sameValue(DataView.prototype.getFloat16.length, 1);

verifyNotEnumerable(DataView.prototype.getFloat16, "length");

verifyNotWritable(DataView.prototype.getFloat16, "length");

verifyConfigurable(DataView.prototype.getFloat16, "length");

verifyProperty(DataView.prototype.getFloat16, "length", {

value: 1,

enumerable: false,

writable: false,

configurable: true,

});

I just copied these over from getFloat32. Would you prefer to use verifyProperty even if it means inconsistency between getFloat16 and all the other dataview methods? Or I can update the other dataview methods as part of this PR and achieve consistency that way, alternatively.

I mean, if you feel like doing the extra cleanup work to update the other dataview methods, great! That would be very kind of you and much appreciated. But it's not a requirement as far as I'm concerned.

I do prefer that we don't introduce new tests using APIs that we're trying to phase out, even if it means temporary inconsistency. If we preferred consistency, it'd mean more and more work for the person eventually doing the cleanup.

Opened #4023. I'll update whichever PR lands second to match the other.

ptomato · 2024-03-14T00:48:41Z

test/built-ins/DataView/prototype/getFloat16/not-a-constructor.js

+
+assert.throws(TypeError, () => {
+  let dv = new DataView(new ArrayBuffer(16)); new dv.getFloat16(0, 0);
+}, '`let dv = new DataView(new ArrayBuffer(16)); new dv.getFloat16(0, 0)` throws TypeError');


Nitpick: I know the other not-a-constructor tests have these autogenerated assert messages, but let's not add more 😝
Could I suggest "DataView.prototype.getFloat16 should not be a constructor" and "Constructing DataView.prototype.getFloat16 should throw TypeError"?

Sure. Would you also like me to update the tests I copied this from?

Opened #4024.

ptomato · 2024-03-14T00:49:22Z

test/built-ins/DataView/prototype/getFloat16/resizable-buffer.js

+assert.sameValue(
+  typeof ArrayBuffer.prototype.resize,
+  'function',
+  'implements ArrayBuffer.prototype.resize'
+);


Is this necessary if we have the feature flag?

I just copied this from the existing getFloat32 test. I don't think it's necessary but I would prefer the new tests match existing ones.

syg

Correctness lgtm. I don't have much of an opinion on the style of these tests.

ptomato

I finished the rest of the review. All good with me, and many thanks for doing this. As for verifyProperty, I'd prefer if we could use verifyProperty in the new tests but I wouldn't block on it.

ptomato · 2024-03-20T23:40:36Z

test/built-ins/DataView/prototype/getFloat16/this-has-no-dataview-internal.js

+assert.throws(TypeError, function() {
+  getFloat16.call({});
+}, "{}");
+
+assert.throws(TypeError, function() {
+  getFloat16.call([]);
+}, "[]");
+
+var ab = new ArrayBuffer(1);
+assert.throws(TypeError, function() {
+  getFloat16.call(ab);
+}, "ArrayBuffer");
+
+var ta = new Int8Array();
+assert.throws(TypeError, function() {
+  getFloat16.call(ta);
+}, "TypedArray");


Just as a note to myself on future reads of this file and the following one, it's OK that the .call() calls don't have plausible arguments because ToIndex(undefined) is 0.

(It's a common pattern elsewhere in the test corpus that these kinds of tests for RequireInternalSlot throwing forget to provide arguments, and in some of those cases the TypeArray might be coming from the missing argument if the implementation fails to include the test for the wrong receiver. That's not the case here. If you feel like adding them for readability, that's fine with me; also fine if you don't.)

ptomato · 2024-03-20T23:53:17Z

test/built-ins/DataView/prototype/setFloat16/set-values-little-endian-order.js

+var result;
+
+result = sample.setFloat16(0, 42, true); // 01010001 01000000
+assert.sameValue(result, undefined, "returns undefined #1");


It seems sufficient to test that the return value is undefined in the following test, which is specifically for that purpose. (However, fine to just leave this in. Not a blocker.)

ptomato · 2024-03-25T19:34:23Z

@bakkot Many thanks for #4023 and #4024. I guess this is ready to land after rebasing, then?

bakkot · 2024-03-25T19:46:42Z

@ptomato Should be. I rebased and re-ran my scripts from those PRs to make the same changes here.

ptomato

It's time then! Really appreciate the extra cleanup work as well.

harness/byteConversionValues.js

rgret-dev · 2024-11-09T19:22:10Z

test/built-ins/Math/f16round/not-a-constructor.js

+assert(!isConstructor(Math.f16round), "Math.f16round is not a constructor");
+
+assert.throws(TypeError, function () {
+  new Math.fround();


I just noticed this while working on the Ladybird browser project, isn't this supposed to be new Math.f16round() instead of new Math.fround()?

Yes, good catch. Do you want to make a PR?

Sure! If someone else beats me to it, no issue either way.

bakkot requested review from a team as code owners March 12, 2024 18:42

bakkot force-pushed the more-float16-cases branch 2 times, most recently from 6233b23 to 0175d96 Compare March 13, 2024 06:32

syg reviewed Mar 13, 2024

View reviewed changes

test/built-ins/Math/f16round/value-convertion.js Show resolved Hide resolved

ptomato reviewed Mar 14, 2024

View reviewed changes

syg approved these changes Mar 19, 2024

View reviewed changes

ptomato approved these changes Mar 21, 2024

View reviewed changes

This was referenced Mar 23, 2024

rewrite some tests to use verifyProperty #4023

Merged

remove some auto-generated error messages in "assert.throws" tests #4024

Merged

bakkot added 5 commits March 25, 2024 12:44

add more cases for float16 coercions

9ea9d02

tests for Math.f16round

155b281

add tests for get/setFloat16

5b93de5

fix typo in test name

7b1829d

re-run rewrite scripts

1d0670c

bakkot force-pushed the more-float16-cases branch from b37e7f3 to 1d0670c Compare March 25, 2024 19:45

ptomato approved these changes Mar 25, 2024

View reviewed changes

ptomato merged commit decf9f2 into main Mar 25, 2024

ptomato deleted the more-float16-cases branch March 25, 2024 22:22

ljharb added a commit to es-shims/Math.f16round that referenced this pull request Mar 27, 2024

[Tests] add extra coverage from tc39/test262#4017

d3caecf

ljharb reviewed Mar 27, 2024

View reviewed changes

harness/byteConversionValues.js Show resolved Hide resolved

harness/byteConversionValues.js Show resolved Hide resolved

harness/byteConversionValues.js Show resolved Hide resolved

harness/byteConversionValues.js Show resolved Hide resolved

ljharb added a commit to es-shims/Math.f16round that referenced this pull request Mar 27, 2024

[Tests] add extra coverage from tc39/test262#4017

5934c2e

bakkot mentioned this pull request Mar 28, 2024

Stage 4 tracking tc39/proposal-float16array#7

Closed

10 tasks

rgret-dev reviewed Nov 9, 2024

View reviewed changes

rgret-dev mentioned this pull request Nov 12, 2024

Fix the not-a-constructor test for Math.f16round() #4323

Merged

-assert.sameValue(DataView.prototype.getFloat16.length, 1);
-verifyNotEnumerable(DataView.prototype.getFloat16, "length");
-verifyNotWritable(DataView.prototype.getFloat16, "length");
-verifyConfigurable(DataView.prototype.getFloat16, "length");
+verifyProperty(DataView.prototype.getFloat16, "length", {
+  value: 1,
+  enumerable: false,
+  writable: false,
+  configurable: true,
+});

Conversation

bakkot commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syg left a comment

Choose a reason for hiding this comment

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptomato commented Mar 25, 2024

Uh oh!

bakkot commented Mar 25, 2024

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

bakkot commented Mar 12, 2024 •

edited

Loading