Skip to content

[import-bytes] add initial tests for import bytes proposal#4648

Merged
ptomato merged 17 commits intotc39:mainfrom
styfle:styfle/proposal-import-bytes
Feb 16, 2026
Merged

[import-bytes] add initial tests for import bytes proposal#4648
ptomato merged 17 commits intotc39:mainfrom
styfle:styfle/proposal-import-bytes

Conversation

@styfle
Copy link
Copy Markdown
Member

@styfle styfle commented Nov 9, 2025

@styfle styfle requested review from a team as code owners November 9, 2025 20:53
import value from './bytes-from-empty-FIXTURE.bin' with { type: 'bytes' };

assert(value instanceof Uint8Array);
assert(value.buffer instanceof ArrayBuffer);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

there should be some kind of assertion that it's immutable (not sure what that'd look like tho)

same throughout

Copy link
Copy Markdown
Member Author

@styfle styfle Dec 22, 2025

Choose a reason for hiding this comment

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

The lines below are checking for immutable (asserts that an error is thrown on resize and also on transfer)

assert.throws(TypeError, function() {
  value.buffer.resize(0);
});

assert.throws(TypeError, function() {
  value.buffer.transfer();
});

Let me know if there is a better way to check for immutability.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

@styfle styfle Dec 26, 2025

Choose a reason for hiding this comment

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

Interesting. I don't see any existing tests for ArrayBuffer.prototype.immutable.

I was basing this new test on the existing tests here:

var ab = (new ArrayBuffer(4)).transferToImmutable();
assert.throws(TypeError, function() {
ab.transfer();
});

var ab = (new ArrayBuffer(4)).transferToImmutable();
assert.throws(TypeError, function() {
ab.resize(0);
});

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I went ahead and added the new assertion in cafcddc

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting. I don't see any existing tests for ArrayBuffer.prototype.immutable.

Still in PR, alas.

@ptomato
Copy link
Copy Markdown
Contributor

ptomato commented Nov 10, 2025

Note, the empty file is failing because fixture files need to contain the string _FIXTURE and these have a dash where they should have an underscore: https://github.com/tc39/test262/blob/main/INTERPRETING.md#modules

@styfle styfle mentioned this pull request Dec 22, 2025
8 tasks
@styfle
Copy link
Copy Markdown
Member Author

styfle commented Dec 22, 2025

@ptomato Thanks! I updated the fixtures to use underscores and that solved the Test262File: missing "contents" error.

However, I ran into another error.

  File "/home/runner/work/test262/test262/tools/generation/lib/test.py", line 29, in load
    self.source = handle.read()
                  ~~~~~~~~~~~^^
  File "<frozen codecs>", line 325, in decode
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x89 in position 0: invalid start byte

So I pushed some changes to the python code to ignore binary files.

I see there are still a few engines on circleci that are failing. Please let me know how to proceed, thanks!

@ptomato
Copy link
Copy Markdown
Contributor

ptomato commented Jan 20, 2026

@styfle instead of hardcoding binary extensions I think it'd be better to just skip cleaning any files with _FIXTURE in the name? Would that work for you?

The CI jobs are timing out because they are detecting hundreds of tests as different in this PR than from main. Probably if you rebase on the latest state of the main branch, that'll go away.

@styfle styfle force-pushed the styfle/proposal-import-bytes branch from cafcddc to 25407a1 Compare January 22, 2026 02:46
@styfle
Copy link
Copy Markdown
Member Author

styfle commented Jan 22, 2026

@ptomato Thanks! I rebased on main and force pushed. That seemed to fix CI ✅

I also applied your suggestion to skip files with _FIXTURE in 7d42a35.

Let me know how that looks, thanks!

@styfle styfle requested review from bakkot and ljharb January 22, 2026 03:03
Copy link
Copy Markdown
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

👍 on the modifications to the test generator, everything else generally looks good.

assert.sameValue(value.buffer.byteLength, 16);
assert.sameValue(value.buffer.immutable, true);

assert.compareArray(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will fail, no? Comments won't be stripped from the JS file?

Copy link
Copy Markdown
Member Author

@styfle styfle Jan 23, 2026

Choose a reason for hiding this comment

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

The comments are only here for the human reader to make it clear that each byte represents a character in the js file that was imported, in this case:

var foo = 'bar'

It should work just fine. Am I misunderstanding you?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By my reading, the imported byte array should contain the UTF-8 encoding of

// Copyright (C) 2025 @styfle. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
var foo = 'bar'

and not

var foo = 'bar'

but this test expects the latter.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I understand now, thanks!

You're right, I pushed commit fdc288c to remove it but then lint fails.

Is there a recommended way to disable lint on the FIXTURE files?

Copy link
Copy Markdown
Member Author

@styfle styfle Jan 25, 2026

Choose a reason for hiding this comment

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

I think I figured it out. I pushed ac5338b to disable license checks on files containing _FIXTURE in the name.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be nicer to go the other way and include the bytes of the comment here instead of messing with the lint rules.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I considered that but I figured I would fix it for everyone since fixture files are not source code.

It seems like this was the first fixture file to be js perhaps?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On the contrary, fixture files are source code and this is not the first JS fixture file. JS fixture files are used extensively in other importing tests.

I think the best course of action is to keep the license header and include the bytes of the comment in the test, as annoying as that may be.

(You could say that it's ridiculous to license var foo = 'bar' and you would be right, but we don't want to get into the weeds of "is this JS file big enough to merit a license header". All JS files just need to have either the license header or the public domain declaration.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok I reverted those commits and pushed a change to read all the bytes 👍

Please take another look.

@styfle styfle requested a review from ptomato January 23, 2026 21:12
Copy link
Copy Markdown
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Thanks!

It occurs to me that these tests will fail if checked out on Windows with git configured for core.autocrlf = true. (The 0x0a bytes will get replaced with 0x0d 0x0a on disk)

Do we need to specify in INTERPRETING.md that test262 consumers must not have core.autocrlf = true in their gitconfig?

@styfle
Copy link
Copy Markdown
Member Author

styfle commented Jan 28, 2026

@ptomato Sounds good. I added a note in c7ec727.

Is there anyone else that needs to approve before this gets merged?

@ptomato
Copy link
Copy Markdown
Contributor

ptomato commented Jan 28, 2026

Not really. The rule of thumb I've been trying to stick to is: if it's small, or has 2 reviews already, or is urgent for plenary, I merge it; otherwise, I leave a couple of days to see if anyone else comments.

@ptomato
Copy link
Copy Markdown
Contributor

ptomato commented Feb 3, 2026

Some referenced fixture files appear to be missing from the commit

lies

The length assertion in bytes-from-js.js might need a quick adjustment

Seems to be legit, @styfle PTAL?

@ptomato
Copy link
Copy Markdown
Contributor

ptomato commented Feb 6, 2026

Some referenced fixture files appear to be missing from the commit

lies

The length assertion in bytes-from-js.js might need a quick adjustment

Seems to be legit, @styfle PTAL?

What is it with these LLM review bots that randomly pop up, leave review comments, and then after you reply to them the comment gets deleted?? It looks like I'm talking to myself.

In any case, the above was my reply to a review bot that made 2 suggestions, one of them nonsense, the other one was a legit comment about the length assertion in bytes-from-js.js not being updated to match the new length of the fixture file.

@styfle
Copy link
Copy Markdown
Member Author

styfle commented Feb 14, 2026

@ptomato Good call! Updated in 73d12de

@styfle
Copy link
Copy Markdown
Member Author

styfle commented Feb 14, 2026

@ljharb @bakkot Any other feedback?

@ptomato
Copy link
Copy Markdown
Contributor

ptomato commented Feb 16, 2026

I think it's fine to merge it now, it's been up for several weeks since I approved it.

@ptomato ptomato force-pushed the styfle/proposal-import-bytes branch from 73d12de to d98d096 Compare February 16, 2026 17:12
@ptomato ptomato enabled auto-merge (squash) February 16, 2026 17:12
@ptomato ptomato merged commit 3af36be into tc39:main Feb 16, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test plan for import bytes

6 participants