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.

Sign up for GitHub

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

Jump to bottom

TextEncoder/TextDecoder SharedArrayBuffer tests #19531

Merged
merged 4 commits into from Oct 9, 2019
Merged

TextEncoder/TextDecoder SharedArrayBuffer tests #19531

merged 4 commits into from Oct 9, 2019

Conversation

Bnaya
Copy link
Contributor

@Bnaya Bnaya commented Oct 4, 2019

Add tests for TextEncoder/TextEncoder with SharedArrayBuffer

@Bnaya Bnaya force-pushed the shared-array-buffer-text-encode-decode branch from f388330 to f29981a Compare October 4, 2019 22:09
@Bnaya Bnaya changed the title TextEncoder/TextEncoder SharedArrayBuffer tests TextEncoder/TextDecoder SharedArrayBuffer tests Oct 4, 2019
This was referenced Oct 4, 2019
Closed
Merged
annevk
annevk reviewed Oct 7, 2019
View reviewed changes
encoding/encodeInto.any.js Outdated Show resolved Hide resolved
encoding/encodeInto.any.js Outdated Show resolved Hide resolved
encoding/encodeInto.any.js Outdated Show resolved Hide resolved
encoding/streams/decode-utf8.any.js Outdated Show resolved Hide resolved
@Bnaya Bnaya force-pushed the shared-array-buffer-text-encode-decode branch 3 times, most recently from 37961aa to 1df43ba Compare October 7, 2019 11:02
annevk
annevk reviewed Oct 7, 2019
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

@hsivonen do you have any other tests you'd like to see added/modified? (Can also be done incrementally in a new PR perhaps.)

encoding/encodeInto.any.js Outdated Show resolved Hide resolved
encoding/encodeInto.any.js Outdated Show resolved Hide resolved
@Bnaya Bnaya force-pushed the shared-array-buffer-text-encode-decode branch from 1df43ba to 5b40312 Compare October 7, 2019 15:17
annevk
annevk reviewed Oct 7, 2019
View reviewed changes
encoding/encodeInto.any.js Outdated Show resolved Hide resolved
@hsivonen
Copy link
Member

hsivonen commented Oct 8, 2019

do you have any other tests you'd like to see added/modified? (Can also be done incrementally in a new PR perhaps.)

I think it's enough to have SharedArrayBuffer testing with some contents. At least the initial implementations will likely be indifferent to actual content.

If implementations ever want to do fancy optimizations, it would be good to test things like a stretch of more than 16 ASCII bytes followed by UTF-8 sequences with a variety of lead bytes followed by buffer end, but the kind of failure modes triggered would be very timing-dependent anyway and not really testable.

In summary, I think there's no need to add anything specific regarding buffer content at this point.

@Bnaya Bnaya force-pushed the shared-array-buffer-text-encode-decode branch from c506534 to e033767 Compare October 8, 2019 21:55
annevk
annevk approved these changes Oct 9, 2019
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks good to me, module some style nits I may or may not care enough to fix myself. @ricea any feedback?

@ricea
Copy link
Contributor

ricea commented Oct 9, 2019

This looks good to me, module some style nits I may or may not care enough to fix myself. @ricea any feedback?

Looks good.

@annevk annevk merged commit a910ad1 into web-platform-tests:master Oct 9, 2019
annevk added a commit to whatwg/encoding that referenced this pull request Oct 9, 2019
@annevk
Allow SharedArrayBuffer
c1ad763 
Tests: web-platform-tests/wpt#19531.

Fixes #172.
@Bnaya Bnaya deleted the shared-array-buffer-text-encode-decode branch October 9, 2019 10:25
@Bnaya
Copy link
Contributor Author

Bnaya commented Oct 9, 2019

If the repo had prettier, editor config, eslint or so,
That would have made contributing abit more friendly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ricea ricea ricea left review comments

@annevk annevk annevk approved these changes

@inexorabletash inexorabletash Awaiting requested review from inexorabletash

Assignees

@annevk annevk

Labels
encoding
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Bnaya @hsivonen @ricea @annevk @wpt-pr-bot