Open
Conversation
Contributor
|
It'd be helpful for reviews if you could share more details about your AI process, e.g. are these verbatim output from an LLM, or did you write them and have the LLM review them? |
Member
Author
Yes, I'm happy to! In hindsight, I should've included that originally. First I gave it some background, explained the goal, and asked it to make a test plan based on the combination of existing iterator helper tests and the test suite from the proposal.This is test262, the official conformance test suite for the JavaScript programming language. We are adding tests for a new proposal, Iterator Chunking. The Iterator Chunking proposal adds two new methods to Iterator.prototype: chunks and windows. I have a clone of the Iterator Chunking proposal at /Users/michael/projects/proposal-iterator-chunking, which has an existing test suite in test/index.mjs which tests the polyfill in src/index.ts. Existing test262 tests for Iterator.prototype methods live in the test/built-ins/Iterator/prototype/ directory of this repo. They should be used as inspiration when filling out the test suite. Additionally, we should make sure we cover at least everything that is tested by the proposal repo's test suite. test262 tests are written across many files, and each file tests only one very narrow aspect of the feature. Take a look at these files and make a detailed test plan, including every assertion that would be made. After that, pause so I can review and modify/approve the test plan. Then I asked it to help me confirm that we have at least covered the whole test suite from the proposal in our plan. It added one more test after finding an assertion that was not covered.Let's take another look at the iterator chunking proposal's existing test suite. Annotate each assertion in that file with a comment referencing the test262 file in the proposed testing plan that will make the same or equivalent assertion. If there's no such test in the testing plan, add one. Write out the testing plan to a markdown file in /tmp. Then I asked it to deduplicate the tests in the plan.Alright, let's review the testing plan one more time. Are there any tests that duplicate or obviate another test? Is this the minimum number of tests that we need to validate the entire feature? It found like 5 or 6 opportunities. I told it to keep one specific to coercion that it was going to eliminate.Yes, but keep the no-coercion tests separate. Even if they're duplicative, it's important to call out that unique behaviour. I told it to annotate the existing test suite again.Let's take another look at the iterator chunking proposal's existing test suite. Annotate each assertion in that file with a comment referencing the test262 file in the proposed testing plan that will make the same or equivalent assertion. If there's no such test in the testing plan, add one. Here's the final plan it prepared: https://gist.github.com/michaelficarra/10f7824706aefb6f0959205015e9ee68#file-iterator-chunking-test-plan-md Then I told it to write the tests following the plan.Let's execute on the test plan. Create each of the planned files and populate them with the appropriate test assertions. Make sure each test is gated on the iterator-chunking feature. Add iterator-chunking to features.txt. Where appropriate, include relevant snippets of the algorithm steps from spec.emu from the proposal repo. For each test file, look in the proposal repo's test file to see if there's an analagous assertion, and if there is, match it as closely as possible. Attribute the file copyright at the top to 2026 Michael Ficarra. Include a detailed description and the relevant emu-clause ID (as esid) in the yaml metadata header at the top of every file. I committed that and asked it to update the tests for tc39/proposal-iterator-chunking#30I made a change to the proposal. Now, for chunk and window sizes that are non-Numbers or non-integral Numbers, we throw a TypeError before checking whether they're in range and throwing a RangeError. Make the corresponding change to any tests that currently assert RangeError for these kinds of inputs. Then I pushed and reviewed the whole diff on GitHub. I found the following issues which I corrected with fixups (I probably should have kept the commits):
Then I opened the PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds tests for https://github.com/tc39/proposal-iterator-chunking. Includes a second commit that assumes tc39/proposal-iterator-chunking#30 will be merged. We can back it out or modify it if that PR is rejected or modified.
Disclaimer: I used AI in the process of creating this PR.