Frontmatter
| title | test(util): add unit coverage for Neo.util.Array (#15124) |
| author | msranjana |
| state | Open |
| createdAt | 4:18 AM |
| updatedAt | 4:47 AM |
| closedAt | |
| mergedAt | |
| branches | dev ← dev |
| url | https://github.com/neomjs/neo/pull/15343 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The ticket asks for one focused, test-only spec in the established util-unit location. The PR does exactly that, without changing runtime behavior or inventing a new abstraction.
Peer-Review Opening: Thanks for taking this newcomer ticket and covering the utility contract so thoroughly. The change is focused, readable, and lands in the right place.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #15124; the changed-file list; current src/util/Array.mjs; established test/playwright/unit/util sibling specs; the ticket Contract Ledger.
- Expected Solution Shape: One focused Playwright unit spec at test/playwright/unit/util/Array.spec.mjs, covering all 11 public helpers plus mutation/reference, scalar/array, ordering, toggle, and set-like input-immutability contracts. Runtime source must remain untouched.
- Patch Verdict: Matches the expected shape. The diff adds only the canonical spec, covers every named helper and branch family, and preserves the ticket's test-only boundary.
- Premise Coherence: Coheres with verify-before-assert by converting a widely consumed utility's existing behavior into executable evidence; no swarm, identity, or architectural-value surface is changed.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #15124
- Related Graph Nodes: Neo.util.Array; unit-testing; contributor-experience
🔬 Depth Floor
Documented search: I actively looked for missing public helpers, mutator assertions that failed to prove original-reference return, absent scalar/array branches, structurally-equal-object identity mistakes, the counterintuitive unshift ordering, and accidental set-like input mutation, and found no concerns.
Rhetorical-Drift Audit (per guide §7.4):
Findings: N/A — routine unit coverage with no architectural prose.
🧠 Graph Ingestion Notes
- [KB_GAP]: None.
- [TOOLING_GAP]: None in the submitted change.
- [RETROSPECTIVE]: A focused test-only change can pin the full behavior surface of this foundational helper without coupling tests to consumers.
🎯 Close-Target Audit
- Close-target identified: #15124
- #15124 is confirmed not epic-labeled.
Findings: Pass.
📑 Contract Completeness Audit
- #15124 contains a Contract Ledger matrix.
- The test diff matches the ledger exactly: mutation/reference identity, scalar/array branches, ordering, toggle behavior, and non-mutating set-like helpers are all pinned without runtime drift.
Findings: Pass.
N/A Audits — 🪜 📡 🔗
N/A across listed dimensions: all close-target ACs are unit-testable, and this PR changes no OpenAPI, skill, convention, or cross-substrate surface.
🧪 Test-Evidence & Location Audit
- Execution evidence: exact-head required CI green (8/8 required checks passed) at 695f5b5620753b0cef36e39b5c0ec1d33c9297ec; author receipt reports 35/35 focused tests.
- Reviewer falsifier: npm run test-unit -- test/playwright/unit/util/Array.spec.mjs — 35/35 passed at the exact PR head while exercising the named behavior families.
- Test location: pass — test/playwright/unit/util/Array.spec.mjs is the canonical sibling location.
Findings: Pass once [HOSTED_GATE] is replaced by the observed exact-head result.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
- [ARCH_ALIGNMENT]: 100 - Test-only work lands in the established util-unit path and leaves runtime boundaries intact.
- [CONTENT_COMPLETENESS]: 100 - All 11 helpers and every ticketed contract family are covered.
- [EXECUTION_QUALITY]: 100 - Clear arrangement, exact reference assertions, and meaningful edge cases.
- [PRODUCTIVITY]: 100 - One focused file closes the entire bounded ticket without scope expansion.
- [IMPACT]: 82 - Pins a foundational utility used across many Neo body surfaces.
- [COMPLEXITY]: 28 - Deterministic unit coverage with several subtle mutation and ordering contracts.
- [EFFORT_PROFILE]: Quick Win - High confidence and repository value from a compact, test-only change.
Excellent first contribution—thank you.
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The change is exactly the bounded shape #15124 called for: one canonical util spec, no runtime mutation, and direct coverage of the existing public helper contract. There is no premise failure, architecture debt, or follow-up work to manufacture.
Peer-Review Opening: Welcome to Neo.mjs, Ranjana — this is a strong first contribution. The spec is focused, readable, and unusually complete for a small utility surface: it covers the behavior that matters without turning the test into a second implementation.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Live #15124 and its Contract Ledger; exact changed-file list; current src/util/Array.mjs; the canonical unit-test skill and runner; sibling util specs; exact head 695f5b5620753b0cef36e39b5c0ec1d33c9297ec; and hosted check state.
- Expected Solution Shape: One test-only spec under test/playwright/unit/util/ using Neo's setup, namespace augmentation, and direct static-helper calls. It must pin all 11 public helpers, mutation/reference semantics, scalar and array inputs, ordering, toggle branches, set-like immutability, and defaults without changing runtime code.
- Patch Verdict: Matches. The diff is one 305-line spec and no production file; every public helper is exercised through isolated per-test arrays using the established setup/import pattern.
- Premise Coherence: Coheres with verify-before-assert: the contribution converts a broad, heavily consumed utility contract into executable evidence while preserving the existing runtime as the source of truth.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #15124
- Related Graph Nodes: Neo.util.Array; unit-test; contributor-experience; core utility contracts
🔬 Depth Floor
Documented search: I actively looked for a skipped public helper, a wrong Neo unit-test bootstrap/import shape, an unpinned return-reference or input-mutation contract, and edge gaps around removeAdd array removal and object-duplicate handling and found no merge-gating concern. Those final two mechanics are already discriminated by the direct remove/add/hasItem controls rather than being silently absent.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: the all-11-helper and 35/35 claims match the exact diff and reviewer execution.
- Anchor & Echo summaries: N/A — no production JSDoc or architectural prose changed.
-
[RETROSPECTIVE]tag: N/A — none added. - Linked anchors:
Closes #15124is the correct non-epic leaf and follows the maintainer's contributor guidance.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: GitHub initially marked the fork workflowsaction_required; this was the normal first-time-contributor approval gate, not a branch failure. A maintainer approved the five runs and the resulting hosted checks are green.[RETROSPECTIVE]: A pure helper with hundreds of downstream call sites is excellent contributor work when the ticket supplies the behavioral ledger and the spec pins mutation, identity, ordering, defaults, and immutability directly.
N/A Audits — 📑 🪜 📡 🔗
N/A across listed dimensions: this is a test-only contribution that changes no public runtime contract, beyond-CI evidence class, MCP description, or cross-skill convention.
🎯 Close-Target Audit
- Close-target identified: #15124.
- #15124 is open, assigned to the contributor, and not epic-labeled.
- The commit and PR both use the same correctly scoped close target.
Findings: Pass.
🧪 Test-Evidence & Location Audit
- Execution evidence: all 8 exact-head hosted checks are green at
695f5b5620753b0cef36e39b5c0ec1d33c9297ec, including the complete unit job. Reviewer exact-head focused run: 35/35 passed with Neo's custom unit configuration. - Reviewer falsifier: manual contract-to-assertion mapping confirmed all 11 public helpers and every #15124 ledger axis: original-array identity, scalar/array inputs, duplicates, object identity, ordering, both toggle paths, defaults, and set-like input immutability.
- Test location: Pass —
test/playwright/unit/util/Array.spec.mjsis the canonical source-mirrored util location.
Findings: Pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 100 - Exact canonical test placement, correct Neo bootstrap, and zero runtime scope expansion.[CONTENT_COMPLETENESS]: 100 - All 11 helpers and every ticket-ledger axis are covered.[EXECUTION_QUALITY]: 100 - Exact-head focused 35/35 and all hosted checks are green; assertions isolate each behavior cleanly.[PRODUCTIVITY]: 100 - The contribution fully closes the bounded good-first issue without follow-up debt.[IMPACT]: 72 - The change is test-only, but it protects a foundational utility used across core consumers.[COMPLEXITY]: 32 - A broad but deterministic static-helper surface with no async, rendering, or lifecycle coupling.[EFFORT_PROFILE]: Maintenance - Focused regression coverage for a mature core utility.
Excellent first contribution. The PR is ready for the human merge step.
[review-budget-managed]
— Euclid (@neo-gpt) · OpenAI GPT-5.6 Sol Ultra
Second maintainer read — @neo-gpt's approval already stands and this is green, so this isn't a gate. It's here because this is a notably good first contribution and the reasons are specific rather than polite.
Closes #15124
Problem
Neo.util.Arrayis a foundational utility used throughout Neo's VDOM, observable, component, layout, grid, selection, and docking code. Despite its widespread use, it did not have a dedicated unit test suite.Change
This PR adds a focused Playwright unit test suite at:
The suite provides comprehensive coverage for all 11 public static helper methods.
addunshifthasItemremoveremoveAddtoggletrue/falseoverride, same-reference returninsertmovefromIndexclampingdifferenceintersectionunionTest Results
Out of Scope
src/util/Array.mjsor any runtime code.You tested two different contracts, and you got both right
That's the thing most coverage PRs miss, and it's the reason these 35 tests would actually catch a regression instead of just executing the file:
add/remove/insert/unshift/removeAddmutate in place — you asserted "return the same reference" on every one of them, consistently.unionis[...new Set(concat(...arguments))]— a new array. You did not copy the same-reference assertion across; you asserted "should not mutate the input arrays" instead.Two contracts, two invariants, no copy-paste between them. That distinction only exists if you read
src/util/Array.mjsrather than exercising its surface — and it's exactly the kind of thing a future refactor would break silently.Three tests that will earn their keep
should return false for structurally equal but distinct objects— the subtle one.hasItemis reference identity, and this pins it. The day someone "improves" it to a deep-equal or aJSON.stringifycompare, this test is what stops them.should add … (default branch)/should remove … (default branch)/should add when the explicit add parameter is true regardless of membership—toggle(arr, item, add = !this.hasItem(arr, item))has a computed default and an explicit override, and you covered both sides of both. Default-parameter branches are the single most commonly untested thing in a codebase, because they look like they can't have behaviour.should be a no-op for missing removal targets— the boring case that is always the one that breaks.For context on why this lands well here
The maintainers have spent tonight, collectively, dealing with tests that passed for reasons unrelated to what they claimed to prove — witnesses whose setup healed the bug before the probe ran, greens that meant "never executed", assertions pointed at the wrong subject. Coverage that asserts the contract rather than the call is the thing that actually pays. Yours does, and it does it for a file that had no spec at all.
src/util/Array.mjsis small and load-bearing — a lot of the framework moves arrays through it — so having it pinned matters more than the line count suggests.Merge-readiness is @tobiu's call (maintainers here never self-merge), and this is in his queue as green + approved. Nothing needed from you.
Thanks for picking a genuine gap and covering it properly. 🖖
— @neo-opus-grace