Expand typed array coverage across core methods#367
Merged
Conversation
- Add broader TypedArray coverage for constructors and prototype methods - Include numeric, iterator, mutation, and copy semantics checks
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughGeneralizes many TypedArray tests from Int32Array-specific cases to parameterized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
the
spec compliance
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/built-ins/TypedArray/prototype/toString-tag.js (1)
38-55: Unify constructor sources to prevent test drift.Line 46 adds
Float16Arrayhere, but the earlierconstructorslist (Line 2–14) is separate and still excludes it, so descriptor/getter-path assertions are not exercised forFloat16Array. Use one shared constructor table for both sections.♻️ Suggested refactor
describe("TypedArray Symbol.toStringTag", () => { - const constructors = [ - [Int8Array, "[object Int8Array]"], - [Uint8Array, "[object Uint8Array]"], - [Uint8ClampedArray, "[object Uint8ClampedArray]"], - [Int16Array, "[object Int16Array]"], - [Uint16Array, "[object Uint16Array]"], - [Int32Array, "[object Int32Array]"], - [Uint32Array, "[object Uint32Array]"], - [Float32Array, "[object Float32Array]"], - [Float64Array, "[object Float64Array]"], - [BigInt64Array, "[object BigInt64Array]"], - [BigUint64Array, "[object BigUint64Array]"], - ]; + const constructors = [ + [Int8Array, "Int8Array"], + [Uint8Array, "Uint8Array"], + [Uint8ClampedArray, "Uint8ClampedArray"], + [Int16Array, "Int16Array"], + [Uint16Array, "Uint16Array"], + [Int32Array, "Int32Array"], + [Uint32Array, "Uint32Array"], + [Float16Array, "Float16Array"], + [Float32Array, "Float32Array"], + [Float64Array, "Float64Array"], + [BigInt64Array, "BigInt64Array"], + [BigUint64Array, "BigUint64Array"], + ]; constructors.forEach((pair) => { const Ctor = pair[0]; - const tag = pair[1]; + const shortTag = pair[1]; + const tag = "[object " + shortTag + "]"; @@ - const shortTag = tag.slice(8, -1); expect(desc.get.call(new Ctor(0))).toBe(shortTag); @@ - test.each([ - [Int8Array, "Int8Array"], - [Uint8Array, "Uint8Array"], - [Uint8ClampedArray, "Uint8ClampedArray"], - [Int16Array, "Int16Array"], - [Uint16Array, "Uint16Array"], - [Int32Array, "Int32Array"], - [Uint32Array, "Uint32Array"], - [Float16Array, "Float16Array"], - [Float32Array, "Float32Array"], - [Float64Array, "Float64Array"], - [BigInt64Array, "BigInt64Array"], - [BigUint64Array, "BigUint64Array"], - ])("%s has correct @@toStringTag", (TA, expected) => { + test.each(constructors)("%s has correct @@toStringTag", (TA, expected) => { const ta = new TA(0); expect(Object.prototype.toString.call(ta)).toBe("[object " + expected + "]"); expect(ta[Symbol.toStringTag]).toBe(expected); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/TypedArray/prototype/toString-tag.js` around lines 38 - 55, The test added Float16Array to the test.each table but not to the earlier constructors array, causing descriptor/getter-path checks to skip Float16Array; consolidate by creating and exporting/using a single shared constructors collection (e.g., rename the existing constructors array or create a shared const like constructorsList) and reference that same identifier in both the toStringTag test (the test.each([...]) and the earlier descriptor/getter-path assertions) so Float16Array is included in both test sections.tests/built-ins/TypedArray/prototype/symbol-iterator.js (1)
46-59: Optional: add BigInt iterator parity tests in this file.A small
test.each([BigInt64Array, BigUint64Array])block for the same spread/iteration assertions would align this file with other iterator suites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/TypedArray/prototype/symbol-iterator.js` around lines 46 - 59, Add parity tests for BigInt typed arrays by adding a new test.each block for BigInt64Array and BigUint64Array that mirrors the existing checks: create a TA instance with [1n, 2n, 3n], assert that [...ta] equals [1n,2n,3n], and assert that a for-of loop over ta collects [1n,2n,3n]; reference the same test patterns used in the existing describe.each for Int8Array etc., and use the constructors BigInt64Array and BigUint64Array to locate where to add the block.tests/built-ins/TypedArray/prototype/sort.js (1)
73-95: Optional: add a float-specific edge case forsort().Consider one targeted case for
Float16Array/Float32Array/Float64ArrayinvolvingNaN(and optionally-0/+0) to guard numeric ordering edge semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/TypedArray/prototype/sort.js` around lines 73 - 95, Add a targeted test inside the existing describe.each block for Float16Array, Float32Array, and Float64Array that verifies sort() handles NaN (and optionally -0/+0) correctly: locate the describe.each([...]) block and add a new test (e.g., "handles NaN and signed zero") that constructs a TA with values like [NaN, 1, -0, 0, 2] (or similar), calls ta.sort(), and asserts the expected post-sort ordering for that TA subclass (NaN should end up at the end and +0/-0 ordering as per spec) while using the same TA variable name and test harness (test(...), expect(...)) so it integrates with the existing tests for Float16Array/Float32Array/Float64Array.tests/built-ins/TypedArray/prototype/toSorted.js (1)
32-36: Make the non-mutation test fully assertive.Line 35 checks only the first slot, so partial mutations can slip through. Verify all elements remain unchanged after
toSorted().Based on learnings: JavaScript tests should cover happy paths, edge cases, and error cases. Keep tests isolated and grouped by feature/filename.Suggested test tightening
test("does not modify original", () => { const ta = new TA([3, 1, 2]); ta.toSorted(); expect(ta[0]).toBe(3); + expect(ta[1]).toBe(1); + expect(ta[2]).toBe(2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/TypedArray/prototype/toSorted.js` around lines 32 - 36, The non-mutation test for TA.prototype.toSorted only asserts ta[0] and can miss other mutations; update the test that creates const ta = new TA([3, 1, 2]) and calls ta.toSorted() to assert that all elements of ta remain unchanged (e.g., compare the entire contents of ta to [3,1,2] using a full equality check or by converting TA to a plain array), referencing the variable ta and the method toSorted to ensure the original TypedArray is not modified.tests/built-ins/TypedArray/prototype/toReversed.js (1)
35-39: Strengthen immutability assertion beyond the first element.Line 38 only validates index 0; a buggy implementation mutating later slots would still pass. Assert the full array state after
toReversed().Based on learnings: JavaScript tests should cover happy paths, edge cases, and error cases. Keep tests isolated and grouped by feature/filename.Suggested test tightening
test("does not modify original", () => { const ta = new TA([1, 2, 3]); ta.toReversed(); - expect(ta[0]).toBe(1); + expect(ta[0]).toBe(1); + expect(ta[1]).toBe(2); + expect(ta[2]).toBe(3); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/TypedArray/prototype/toReversed.js` around lines 35 - 39, The test for TA.prototype.toReversed only checks ta[0], which can miss mutations to other indices; update the "does not modify original" test that uses TA and ta.toReversed() to assert the entire typed array remains unchanged (same length and same sequence of elements) after calling toReversed() rather than only checking index 0, so any in-place modifications to later slots will fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/built-ins/TypedArray/prototype/sort.js`:
- Around line 73-95: Add a targeted test inside the existing describe.each block
for Float16Array, Float32Array, and Float64Array that verifies sort() handles
NaN (and optionally -0/+0) correctly: locate the describe.each([...]) block and
add a new test (e.g., "handles NaN and signed zero") that constructs a TA with
values like [NaN, 1, -0, 0, 2] (or similar), calls ta.sort(), and asserts the
expected post-sort ordering for that TA subclass (NaN should end up at the end
and +0/-0 ordering as per spec) while using the same TA variable name and test
harness (test(...), expect(...)) so it integrates with the existing tests for
Float16Array/Float32Array/Float64Array.
In `@tests/built-ins/TypedArray/prototype/symbol-iterator.js`:
- Around line 46-59: Add parity tests for BigInt typed arrays by adding a new
test.each block for BigInt64Array and BigUint64Array that mirrors the existing
checks: create a TA instance with [1n, 2n, 3n], assert that [...ta] equals
[1n,2n,3n], and assert that a for-of loop over ta collects [1n,2n,3n]; reference
the same test patterns used in the existing describe.each for Int8Array etc.,
and use the constructors BigInt64Array and BigUint64Array to locate where to add
the block.
In `@tests/built-ins/TypedArray/prototype/toReversed.js`:
- Around line 35-39: The test for TA.prototype.toReversed only checks ta[0],
which can miss mutations to other indices; update the "does not modify original"
test that uses TA and ta.toReversed() to assert the entire typed array remains
unchanged (same length and same sequence of elements) after calling toReversed()
rather than only checking index 0, so any in-place modifications to later slots
will fail the test.
In `@tests/built-ins/TypedArray/prototype/toSorted.js`:
- Around line 32-36: The non-mutation test for TA.prototype.toSorted only
asserts ta[0] and can miss other mutations; update the test that creates const
ta = new TA([3, 1, 2]) and calls ta.toSorted() to assert that all elements of ta
remain unchanged (e.g., compare the entire contents of ta to [3,1,2] using a
full equality check or by converting TA to a plain array), referencing the
variable ta and the method toSorted to ensure the original TypedArray is not
modified.
In `@tests/built-ins/TypedArray/prototype/toString-tag.js`:
- Around line 38-55: The test added Float16Array to the test.each table but not
to the earlier constructors array, causing descriptor/getter-path checks to skip
Float16Array; consolidate by creating and exporting/using a single shared
constructors collection (e.g., rename the existing constructors array or create
a shared const like constructorsList) and reference that same identifier in both
the toStringTag test (the test.each([...]) and the earlier
descriptor/getter-path assertions) so Float16Array is included in both test
sections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe7eb59f-c586-4774-8230-c192861e056f
📒 Files selected for processing (34)
tests/built-ins/TypedArray/from.jstests/built-ins/TypedArray/of.jstests/built-ins/TypedArray/prototype/at.jstests/built-ins/TypedArray/prototype/copyWithin.jstests/built-ins/TypedArray/prototype/entries.jstests/built-ins/TypedArray/prototype/every.jstests/built-ins/TypedArray/prototype/fill.jstests/built-ins/TypedArray/prototype/filter.jstests/built-ins/TypedArray/prototype/find.jstests/built-ins/TypedArray/prototype/findIndex.jstests/built-ins/TypedArray/prototype/findLast.jstests/built-ins/TypedArray/prototype/findLastIndex.jstests/built-ins/TypedArray/prototype/forEach.jstests/built-ins/TypedArray/prototype/includes.jstests/built-ins/TypedArray/prototype/indexOf.jstests/built-ins/TypedArray/prototype/join.jstests/built-ins/TypedArray/prototype/keys.jstests/built-ins/TypedArray/prototype/lastIndexOf.jstests/built-ins/TypedArray/prototype/map.jstests/built-ins/TypedArray/prototype/reduce.jstests/built-ins/TypedArray/prototype/reduceRight.jstests/built-ins/TypedArray/prototype/reverse.jstests/built-ins/TypedArray/prototype/set.jstests/built-ins/TypedArray/prototype/slice.jstests/built-ins/TypedArray/prototype/some.jstests/built-ins/TypedArray/prototype/sort.jstests/built-ins/TypedArray/prototype/subarray.jstests/built-ins/TypedArray/prototype/symbol-iterator.jstests/built-ins/TypedArray/prototype/toReversed.jstests/built-ins/TypedArray/prototype/toSorted.jstests/built-ins/TypedArray/prototype/toString-tag.jstests/built-ins/TypedArray/prototype/toString.jstests/built-ins/TypedArray/prototype/values.jstests/built-ins/TypedArray/prototype/with.js
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 216 improved · 🔴 66 regressed · 104 unchanged · avg +2.8% arraybuffer.js — Interp: 🟢 7, 🔴 3, 4 unch. · avg +1.4% · Bytecode: 🟢 7, 7 unch. · avg +3.3%
arrays.js — Interp: 🟢 17, 2 unch. · avg +7.6% · Bytecode: 🟢 8, 11 unch. · avg +2.4%
async-await.js — Interp: 🟢 2, 4 unch. · avg +1.7% · Bytecode: 🟢 1, 🔴 1, 4 unch. · avg +2.5%
base64.js — Interp: 🟢 9, 1 unch. · avg +6.0% · Bytecode: 🟢 2, 8 unch. · avg +1.3%
classes.js — Interp: 🟢 15, 🔴 4, 12 unch. · avg +2.2% · Bytecode: 🟢 8, 🔴 1, 22 unch. · avg +1.7%
closures.js — Interp: 🟢 10, 1 unch. · avg +6.0% · Bytecode: 🟢 4, 🔴 1, 6 unch. · avg +1.0%
collections.js — Interp: 🟢 6, 6 unch. · avg +2.7% · Bytecode: 🟢 1, 🔴 2, 9 unch. · avg +0.1%
csv.js — Interp: 🟢 12, 1 unch. · avg +10.1% · Bytecode: 🟢 3, 10 unch. · avg +0.6%
destructuring.js — Interp: 🟢 11, 11 unch. · avg +3.4% · Bytecode: 🟢 8, 14 unch. · avg +1.2%
fibonacci.js — Interp: 🟢 8 · avg +5.3% · Bytecode: 🟢 1, 7 unch. · avg +0.9%
float16array.js — Interp: 🟢 19, 🔴 5, 8 unch. · avg +0.5% · Bytecode: 🟢 5, 🔴 6, 21 unch. · avg -0.2%
for-of.js — Interp: 🟢 1, 6 unch. · avg +2.0% · Bytecode: 🟢 4, 3 unch. · avg +1.3%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 32, 10 unch. · avg +8.4% · Bytecode: 🟢 17, 🔴 4, 21 unch. · avg +1.9%
json.js — Interp: 🟢 2, 🔴 7, 11 unch. · avg -0.7% · Bytecode: 🟢 2, 🔴 6, 12 unch. · avg -2.0%
jsx.jsx — Interp: 🟢 8, 13 unch. · avg +1.9% · Bytecode: 🟢 3, 18 unch. · avg +0.5%
modules.js — Interp: 🟢 6, 3 unch. · avg +5.0% · Bytecode: 🔴 5, 4 unch. · avg -1.3%
numbers.js — Interp: 🟢 3, 🔴 4, 4 unch. · avg -0.0% · Bytecode: 🟢 6, 🔴 1, 4 unch. · avg +3.6%
objects.js — Interp: 🟢 7 · avg +10.0% · Bytecode: 7 unch. · avg -0.2%
promises.js — Interp: 🔴 10, 2 unch. · avg -5.5% · Bytecode: 🟢 1, 🔴 2, 9 unch. · avg -0.7%
regexp.js — Interp: 🟢 11 · avg +14.6% · Bytecode: 🟢 5, 6 unch. · avg +1.6%
strings.js — Interp: 🟢 13, 🔴 3, 3 unch. · avg +0.1% · Bytecode: 🟢 1, 🔴 7, 11 unch. · avg -5.1%
tsv.js — Interp: 🟢 9 · avg +16.1% · Bytecode: 🔴 4, 5 unch. · avg -1.7%
typed-arrays.js — Interp: 🟢 1, 🔴 21 · avg -13.2% · Bytecode: 🟢 15, 🔴 3, 4 unch. · avg +3.3%
uint8array-encoding.js — Interp: 🟢 7, 🔴 9, 2 unch. · avg -1.6% · Bytecode: 🟢 4, 🔴 4, 10 unch. · avg -2.7%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
- Remove overlapping TypedArray coverage across built-in tests - Keep representative cases for edge behavior and callback errors
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/built-ins/TypedArray/prototype/join.js (1)
14-29: Consider adding one error-path join case (single shared test).A small follow-up like
expect(() => ta.join(Symbol())).toThrow(TypeError)would complete happy/edge/error coverage without much extra test weight.Based on learnings: JavaScript tests should cover happy paths, edge cases, and error cases. Keep tests isolated and grouped by feature/filename.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/TypedArray/prototype/join.js` around lines 14 - 29, Add a single shared error-path test to the existing describe.each block that asserts using a Symbol as the separator throws a TypeError: create a new test (e.g., "throws on non-string separator") inside the same describe.each([...]) where TA is available, construct a small TA like new TA([1,2]) and call expect(() => ta.join(Symbol())).toThrow(TypeError), referencing the join method and the TA constructor so the failure case is covered for all typed arrays.tests/built-ins/TypedArray/prototype/filter.js (1)
27-27: Consider centralizing the numeric constructor list into a shared test helper/constant.The inline constructor matrix is likely to repeat across TypedArray prototype suites; extracting it would reduce drift when adding/removing supported constructors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/TypedArray/prototype/filter.js` at line 27, Extract the inline constructor array used in describe.each([...]) into a single exported constant (e.g., NUMERIC_TYPED_ARRAYS or TYPED_ARRAY_CONSTRUCTORS) in a shared test helper module and replace the inline array in this file's describe.each call with that imported constant; update other TypedArray prototype tests to import and use the same constant so all suites (which currently use the TA variable in their describe.each) share one canonical list and avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/built-ins/TypedArray/prototype/filter.js`:
- Line 27: Extract the inline constructor array used in describe.each([...])
into a single exported constant (e.g., NUMERIC_TYPED_ARRAYS or
TYPED_ARRAY_CONSTRUCTORS) in a shared test helper module and replace the inline
array in this file's describe.each call with that imported constant; update
other TypedArray prototype tests to import and use the same constant so all
suites (which currently use the TA variable in their describe.each) share one
canonical list and avoid duplication.
In `@tests/built-ins/TypedArray/prototype/join.js`:
- Around line 14-29: Add a single shared error-path test to the existing
describe.each block that asserts using a Symbol as the separator throws a
TypeError: create a new test (e.g., "throws on non-string separator") inside the
same describe.each([...]) where TA is available, construct a small TA like new
TA([1,2]) and call expect(() => ta.join(Symbol())).toThrow(TypeError),
referencing the join method and the TA constructor so the failure case is
covered for all typed arrays.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a931961d-aa37-4e68-be33-b8f608b220be
📒 Files selected for processing (33)
tests/built-ins/TypedArray/from.jstests/built-ins/TypedArray/of.jstests/built-ins/TypedArray/prototype/at.jstests/built-ins/TypedArray/prototype/copyWithin.jstests/built-ins/TypedArray/prototype/entries.jstests/built-ins/TypedArray/prototype/every.jstests/built-ins/TypedArray/prototype/fill.jstests/built-ins/TypedArray/prototype/filter.jstests/built-ins/TypedArray/prototype/find.jstests/built-ins/TypedArray/prototype/findIndex.jstests/built-ins/TypedArray/prototype/findLast.jstests/built-ins/TypedArray/prototype/findLastIndex.jstests/built-ins/TypedArray/prototype/forEach.jstests/built-ins/TypedArray/prototype/includes.jstests/built-ins/TypedArray/prototype/indexOf.jstests/built-ins/TypedArray/prototype/join.jstests/built-ins/TypedArray/prototype/keys.jstests/built-ins/TypedArray/prototype/lastIndexOf.jstests/built-ins/TypedArray/prototype/map.jstests/built-ins/TypedArray/prototype/reduce.jstests/built-ins/TypedArray/prototype/reduceRight.jstests/built-ins/TypedArray/prototype/reverse.jstests/built-ins/TypedArray/prototype/set.jstests/built-ins/TypedArray/prototype/slice.jstests/built-ins/TypedArray/prototype/some.jstests/built-ins/TypedArray/prototype/sort.jstests/built-ins/TypedArray/prototype/subarray.jstests/built-ins/TypedArray/prototype/symbol-iterator.jstests/built-ins/TypedArray/prototype/toReversed.jstests/built-ins/TypedArray/prototype/toSorted.jstests/built-ins/TypedArray/prototype/toString.jstests/built-ins/TypedArray/prototype/values.jstests/built-ins/TypedArray/prototype/with.js
✅ Files skipped from review due to trivial changes (4)
- tests/built-ins/TypedArray/prototype/find.js
- tests/built-ins/TypedArray/prototype/reduceRight.js
- tests/built-ins/TypedArray/prototype/reverse.js
- tests/built-ins/TypedArray/from.js
🚧 Files skipped from review as they are similar to previous changes (26)
- tests/built-ins/TypedArray/prototype/toString.js
- tests/built-ins/TypedArray/prototype/indexOf.js
- tests/built-ins/TypedArray/prototype/keys.js
- tests/built-ins/TypedArray/prototype/subarray.js
- tests/built-ins/TypedArray/prototype/with.js
- tests/built-ins/TypedArray/prototype/set.js
- tests/built-ins/TypedArray/prototype/copyWithin.js
- tests/built-ins/TypedArray/prototype/findIndex.js
- tests/built-ins/TypedArray/prototype/map.js
- tests/built-ins/TypedArray/prototype/toReversed.js
- tests/built-ins/TypedArray/prototype/at.js
- tests/built-ins/TypedArray/prototype/findLastIndex.js
- tests/built-ins/TypedArray/prototype/entries.js
- tests/built-ins/TypedArray/prototype/every.js
- tests/built-ins/TypedArray/prototype/reduce.js
- tests/built-ins/TypedArray/prototype/some.js
- tests/built-ins/TypedArray/prototype/findLast.js
- tests/built-ins/TypedArray/prototype/forEach.js
- tests/built-ins/TypedArray/prototype/sort.js
- tests/built-ins/TypedArray/prototype/fill.js
- tests/built-ins/TypedArray/prototype/values.js
- tests/built-ins/TypedArray/prototype/slice.js
- tests/built-ins/TypedArray/prototype/symbol-iterator.js
- tests/built-ins/TypedArray/prototype/lastIndexOf.js
- tests/built-ins/TypedArray/of.js
- tests/built-ins/TypedArray/prototype/toSorted.js
Rewrite all TypedArray prototype tests to exercise every behavior across all 10 numeric typed array types via describe.each, rather than only testing Int32Array. This caught a real bug: TypedArray.prototype.sort treated -0 and +0 as equal due to Pascal's comparison operators not distinguishing them. Fix the sort comparison to check sign bits when both values are zero in float typed arrays, ensuring -0 sorts before +0 per spec. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/built-ins/TypedArray/prototype/find.js (1)
3-23: Optional: add an early-exit assertion for callback invocation count.This would tighten
findsemantics coverage by ensuring iteration stops at the first match.Suggested test addition
describe.each([Int8Array, Uint8Array, Uint8ClampedArray, Int16Array, Uint16Array, Int32Array, Uint32Array, Float16Array, Float32Array, Float64Array])("%s", (TA) => { + test("stops at first match", () => { + const ta = new TA([1, 2, 3, 4, 5]); + let calls = 0; + const result = ta.find((x) => { + calls++; + return x === 3; + }); + + expect(result).toBe(3); + expect(calls).toBe(3); + }); + test("returns matching element", () => { const ta = new TA([1, 2, 3, 4]); expect(ta.find(x => x > 2)).toBe(3);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/TypedArray/prototype/find.js` around lines 3 - 23, Add an assertion ensuring the callback is invoked only until the first match to validate early-exit behavior of TypedArray.prototype.find: in the "passes thisArg to callback" test (using TA and ta.find), wrap or replace the callback with a spy/counter (e.g., increment a local counter inside obj.fn or use a stub) and assert after the call that the counter equals 1 (or the expected number) so the iteration stops once the matching element is found; keep the existing thisArg assertion intact.tests/built-ins/TypedArray/prototype/keys.js (1)
15-25: Consider adding an explicit error-path assertion for receiver brand checks.
iterator protocolcoverage is good; adding one non-TypedArray receiver case would complete happy/edge/error coverage in this suite.Proposed test addition
describe.each([Int8Array, Uint8Array, Uint8ClampedArray, Int16Array, Uint16Array, Int32Array, Uint32Array, Float16Array, Float32Array, Float64Array])("%s", (TA) => { @@ test("iterator protocol", () => { const ta = new TA([1, 2]); const iter = ta.keys(); @@ expect(iter.next().done).toBe(true); }); + + test("throws on non-TypedArray receiver", () => { + expect(() => TA.prototype.keys.call({})).toThrow(TypeError); + }); });Based on learnings: JavaScript tests should cover happy paths, edge cases, and error cases, while staying isolated by feature/file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/TypedArray/prototype/keys.js` around lines 15 - 25, Add a negative test asserting that TypedArray.prototype.keys (the iterator produced by TA.prototype.keys / ta.keys()) throws when called with a non-TypedArray receiver: call the keys function with a plain object or an Array instance as the receiver (e.g., Function.prototype.call or .call on TA.prototype.keys) and assert it throws a TypeError; place this alongside the existing "iterator protocol" test to cover receiver brand checks for keys().tests/built-ins/TypedArray/prototype/toReversed.js (1)
2-35: Add receiver validation test to cover error case.The suite validates success paths but lacks an error-case assertion for invalid receivers. The pattern exists in
indexOf.js(test("throws on non-TypedArray receiver", ...)) and should be included here to match spec requirements and the learning guideline to cover edge cases.✅ Suggested test addition
describe.each([Int8Array, Uint8Array, Uint8ClampedArray, Int16Array, Uint16Array, Int32Array, Uint32Array, Float16Array, Float32Array, Float64Array])("%s", (TA) => { + test("throws on invalid receiver", () => { + expect(() => TA.prototype.toReversed.call([])).toThrow(TypeError); + expect(() => TA.prototype.toReversed.call({ 0: 1, length: 1 })).toThrow(TypeError); + }); + test("returns reversed copy", () => { const ta = new TA([1, 2, 3]); const reversed = ta.toReversed();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/TypedArray/prototype/toReversed.js` around lines 2 - 35, Add a negative test that validates receiver checks for TypedArray.prototype.toReversed: create a test named "throws on non-TypedArray receiver" (matching the pattern in indexOf.js) and assert that calling TypedArray.prototype.toReversed with an invalid receiver (e.g., call with {} or [] via .call) throws a TypeError; place this test inside the same describe.each block so it runs for each TA type and references toReversed and the TA constructor to locate where to add it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/built-ins/TypedArray/prototype/find.js`:
- Around line 3-23: Add an assertion ensuring the callback is invoked only until
the first match to validate early-exit behavior of TypedArray.prototype.find: in
the "passes thisArg to callback" test (using TA and ta.find), wrap or replace
the callback with a spy/counter (e.g., increment a local counter inside obj.fn
or use a stub) and assert after the call that the counter equals 1 (or the
expected number) so the iteration stops once the matching element is found; keep
the existing thisArg assertion intact.
In `@tests/built-ins/TypedArray/prototype/keys.js`:
- Around line 15-25: Add a negative test asserting that
TypedArray.prototype.keys (the iterator produced by TA.prototype.keys /
ta.keys()) throws when called with a non-TypedArray receiver: call the keys
function with a plain object or an Array instance as the receiver (e.g.,
Function.prototype.call or .call on TA.prototype.keys) and assert it throws a
TypeError; place this alongside the existing "iterator protocol" test to cover
receiver brand checks for keys().
In `@tests/built-ins/TypedArray/prototype/toReversed.js`:
- Around line 2-35: Add a negative test that validates receiver checks for
TypedArray.prototype.toReversed: create a test named "throws on non-TypedArray
receiver" (matching the pattern in indexOf.js) and assert that calling
TypedArray.prototype.toReversed with an invalid receiver (e.g., call with {} or
[] via .call) throws a TypeError; place this test inside the same describe.each
block so it runs for each TA type and references toReversed and the TA
constructor to locate where to add it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa377fd3-734b-4f77-9a2a-6452f6708196
📒 Files selected for processing (34)
source/units/Goccia.Values.TypedArrayValue.pastests/built-ins/TypedArray/from.jstests/built-ins/TypedArray/of.jstests/built-ins/TypedArray/prototype/at.jstests/built-ins/TypedArray/prototype/copyWithin.jstests/built-ins/TypedArray/prototype/entries.jstests/built-ins/TypedArray/prototype/every.jstests/built-ins/TypedArray/prototype/fill.jstests/built-ins/TypedArray/prototype/filter.jstests/built-ins/TypedArray/prototype/find.jstests/built-ins/TypedArray/prototype/findIndex.jstests/built-ins/TypedArray/prototype/findLast.jstests/built-ins/TypedArray/prototype/findLastIndex.jstests/built-ins/TypedArray/prototype/forEach.jstests/built-ins/TypedArray/prototype/includes.jstests/built-ins/TypedArray/prototype/indexOf.jstests/built-ins/TypedArray/prototype/join.jstests/built-ins/TypedArray/prototype/keys.jstests/built-ins/TypedArray/prototype/lastIndexOf.jstests/built-ins/TypedArray/prototype/map.jstests/built-ins/TypedArray/prototype/reduce.jstests/built-ins/TypedArray/prototype/reduceRight.jstests/built-ins/TypedArray/prototype/reverse.jstests/built-ins/TypedArray/prototype/set.jstests/built-ins/TypedArray/prototype/slice.jstests/built-ins/TypedArray/prototype/some.jstests/built-ins/TypedArray/prototype/sort.jstests/built-ins/TypedArray/prototype/subarray.jstests/built-ins/TypedArray/prototype/symbol-iterator.jstests/built-ins/TypedArray/prototype/toReversed.jstests/built-ins/TypedArray/prototype/toSorted.jstests/built-ins/TypedArray/prototype/toString-tag.jstests/built-ins/TypedArray/prototype/values.jstests/built-ins/TypedArray/prototype/with.js
✅ Files skipped from review due to trivial changes (4)
- tests/built-ins/TypedArray/prototype/at.js
- tests/built-ins/TypedArray/prototype/some.js
- tests/built-ins/TypedArray/of.js
- tests/built-ins/TypedArray/prototype/findLastIndex.js
🚧 Files skipped from review as they are similar to previous changes (25)
- tests/built-ins/TypedArray/from.js
- tests/built-ins/TypedArray/prototype/join.js
- tests/built-ins/TypedArray/prototype/copyWithin.js
- tests/built-ins/TypedArray/prototype/findIndex.js
- tests/built-ins/TypedArray/prototype/forEach.js
- tests/built-ins/TypedArray/prototype/lastIndexOf.js
- tests/built-ins/TypedArray/prototype/toString-tag.js
- tests/built-ins/TypedArray/prototype/set.js
- tests/built-ins/TypedArray/prototype/with.js
- tests/built-ins/TypedArray/prototype/symbol-iterator.js
- tests/built-ins/TypedArray/prototype/every.js
- tests/built-ins/TypedArray/prototype/reduce.js
- tests/built-ins/TypedArray/prototype/toSorted.js
- tests/built-ins/TypedArray/prototype/findLast.js
- tests/built-ins/TypedArray/prototype/fill.js
- tests/built-ins/TypedArray/prototype/indexOf.js
- tests/built-ins/TypedArray/prototype/entries.js
- tests/built-ins/TypedArray/prototype/includes.js
- tests/built-ins/TypedArray/prototype/map.js
- tests/built-ins/TypedArray/prototype/reverse.js
- tests/built-ins/TypedArray/prototype/reduceRight.js
- tests/built-ins/TypedArray/prototype/sort.js
- tests/built-ins/TypedArray/prototype/filter.js
- tests/built-ins/TypedArray/prototype/subarray.js
- tests/built-ins/TypedArray/prototype/values.js
- find.js: assert iteration stops after first match - keys.js: assert TypeError on non-TypedArray receiver - toReversed.js: assert TypeError on non-TypedArray receiver Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
@@toStringTagassertions for all typed array variants, including BigInt typed arrays.