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.

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

add tests for base64 proposal #3994

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

add tests for base64 proposal #3994

wants to merge 8 commits into from

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Jan 22, 2024

Proposal repo.

It's only stage 2, so I'm marking this as a draft for now. I'm hoping to ask for stage 3 at the coming meeting, and that requires "sufficient" test262 tests (but they don't require approval by maintainers, so this review of this PR need to be rushed). Proposal is stage 3, this is ready for review/landing.

@bakkot
Copy link
Contributor Author

bakkot commented Jan 22, 2024

The lint failure is silly - the new "uint8array-base64" feature presumably implies the existence of TypedArrays, so I don't think the "TypedArray" feature should need to be included.

I could do so anyway if it's too much trouble to fix the linter, I guess.

Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

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

Something I noticed while reviewing the spec text for this is that bypassing ECMA-402's GetOption AO (which includes a ToString) and doing the validation manually using is not a String causes string objects to be rejected as option values - while I personally disagree with this choice I think it should be tested either way.

@bakkot
Copy link
Contributor Author

bakkot commented Jan 26, 2024

Sure, I can add tests for that, though in any real implementation I would expect that to be covered by option-coercion tests, which confirm that ToString is not called.

(This behavior is very much intentional.)

@ljharb
Copy link
Member

ljharb commented Jan 26, 2024

@bakkot features are often handled by an exclude list, not an include list, so a runner could exclude typed arrays and not know to exclude this one. it'd be good to add it in explicitly to the test files.

@bakkot
Copy link
Contributor Author

bakkot commented Feb 21, 2024

Marked as ready for review since the proposal is now stage 3.

ljharb added a commit to es-shims/es-arraybuffer-base64 that referenced this pull request Feb 29, 2024
…ict` lastChunkHandling

Also:
 - add test coverage from tc39/test262#3994
 - `fromBase64`: avoid invoking toString on a non-string `string` argument
ljharb added a commit to es-shims/es-arraybuffer-base64 that referenced this pull request Feb 29, 2024
ljharb added a commit to es-shims/es-arraybuffer-base64 that referenced this pull request Feb 29, 2024
…n a non-string `string` argument

Also:
 - add test coverage from tc39/test262#3994
ljharb added a commit to es-shims/es-arraybuffer-base64 that referenced this pull request Feb 29, 2024
… non-string `string` argument

Also:
 - add test coverage from tc39/test262#3994
ljharb added a commit to es-shims/es-arraybuffer-base64 that referenced this pull request Feb 29, 2024
…id invoking toString on a non-string `string` argument

Also:
 - add test coverage from tc39/test262#3994
ljharb added a commit to es-shims/es-arraybuffer-base64 that referenced this pull request Feb 29, 2024
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Added (or verified) all the tests in this PR to my polyfill, which exposed a few minor bugs. LGTM!

@bakkot
Copy link
Contributor Author

bakkot commented Mar 12, 2024

I've addressed comments and hopefully pleased the linter.

@ljharb I see you force-pushed to my branch but I have no idea what changes that included. I'm hoping it's just a rebase but I'm not reading a 10k line diff. I force-pushed over your changes, whatever they were.

@ljharb
Copy link
Member

ljharb commented Mar 12, 2024

@bakkot oh, i just clicked the "rebase branch" button in the PR; i didn't change anything. fine to force push over.

@bakkot
Copy link
Contributor Author

bakkot commented Mar 12, 2024

If you do "update with merge commit" instead of "update with rebase" it'll create a merge commit, which allows git pull --rebase, which is much friendlier. Since the PR will get squashed before merging to main anyway the merge commit will go away eventually.

@ljharb
Copy link
Member

ljharb commented Mar 12, 2024

Merge commits are anathema, even on a PR, so that's not a good idea - git pull --rebase should work fine with a rebased branch.

ljharb added a commit to es-shims/es-arraybuffer-base64 that referenced this pull request Mar 12, 2024
@bakkot
Copy link
Contributor Author

bakkot commented Mar 12, 2024

Sorry, --ff-only, maybe? One of them doesn't work with rebases, I'm pretty sure.

Anyway, the merge commits at least make it easy to tell that it's just an update of the branch, whereas that's not obvious when doing a rebase. Something to discuss elsewhere, though.

@anba
Copy link
Contributor

anba commented Mar 14, 2024

Ideas for additional coverage:

  • Whitespace between and after the padding character =, e.g. Uint8Array.fromBase64("Zg= =") and Uint8Array.fromBase64("Zg= = ").
  • Uint8Array.prototype.setFrom{Base64,Hex} doesn't modify the typed array when the input string is invalid.
  • Typed array is backed by a SharedArrayBuffer.
  • lastChunkHandling option is present, but set to undefined, e.g. the options object is {lastChunkHandling: undefined}.
  • lastChunkHandling option is an invalid string value, e.g. the options object is {lastChunkHandling: "bad"}.
  • alphabet option is present, but set to undefined, e.g. the options object is {alphabet: undefined}.
  • alphabet and lastChunkHandling options are read in the correct order.
  • Extra bits with three element trailing chunk and strict chunk handling, e.g. Uint8Array.fromBase64("ZZZ=", {lastChunkHandling: "strict"}). (The two element chunk case is already covered.)

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.

None yet

5 participants