Cover persistent list implementation gaps with tests - #285
Cover persistent list implementation gaps with tests#285DmitryNekrasov wants to merge 5 commits into
Conversation
| class PersistentListBuilderInsertionTest { | ||
|
|
||
| init { | ||
| check(MAX_BUFFER_SIZE == 32) { "Test sizes assume a trie buffer size of 32" } |
There was a problem hiding this comment.
If we decide to change it, for some reason, how to update the test? I mean, it would be nice to mention it in a comment nearby
There was a problem hiding this comment.
If the buffer size ever changes, the tests will fail on this check, and I'd recommend revisiting all the values manually and carefully verifying that every branch of the code under test is still covered. Automatically deriving all the constants in the tests looks less reliable to me, complicates the test logic and makes it less obvious, and isn't worth it: realistically, we are unlikely to ever change MAX_BUFFER_SIZE.
There was a problem hiding this comment.
Sure, but right now it's unclear how constants were picked and how to choose new values if MAX_BUFFER_SIZE change.
There was a problem hiding this comment.
Done. Documented the picking rules in checkTrieShapeAssumptions()'s KDoc.
ab35d8b to
7575fa6
Compare
6aadeb9 to
a51d05a
Compare
| checkTrieShapeAssumptions() | ||
| } | ||
|
|
||
| private fun ownedBuilderOf(range: IntRange): PersistentList.Builder<Int> { |
There was a problem hiding this comment.
The lower bound is always 0, and with that it's essentially the same function as PersistentListBuilderInsertionTest.ownedBuilderOf.
It might be worth either extracting it into a base class (or into a separate internal utility function).
There was a problem hiding this comment.
Done: extracted ownedBuilderOf(size) as an internal top-level function in listTestUtils.kt.
| return vector | ||
| } | ||
|
|
||
| private fun assertElementsEqual(expected: List<Int>, actual: List<Int>) { |
There was a problem hiding this comment.
kotlin.test.assertContentEquals?
There was a problem hiding this comment.
Done, dropped the helper, call sites use assertContentEquals directly.
| return builder | ||
| } | ||
|
|
||
| private fun assertBuilderContents(expected: List<Int>, builder: PersistentList.Builder<Int>) { |
There was a problem hiding this comment.
It seems like this one is also can be replaced with kotlin.test.assertContentEquals
There was a problem hiding this comment.
Pull request overview
Adds deterministic coverage for persistent-list trie boundaries, iterator traversal, and builder mutation paths.
Changes:
- Tests vector growth, insertion, removal, replacement, and bulk operations.
- Covers mutable iterators and owned versus frozen builder behavior.
- Documents and validates trie-shape boundary assumptions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
TrieIteratorTest.kt |
Tests backward traversal at trie boundaries. |
trieShapeAssumptions.kt |
Documents and validates boundary sizes. |
PersistentVectorTest.kt |
Covers immutable vector operations. |
PersistentListIteratorTest.kt |
Covers persistent and mutable iterators. |
PersistentListBuilderRemovalTest.kt |
Tests builder removal and replacement paths. |
PersistentListBuilderInsertionTest.kt |
Tests builder insertion and trie growth. |
listTestUtils.kt |
Adds an owned-builder test helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Deterministic tests for the persistent list paths that were exercised only by randomized stress runs or not at all, with sizes at the trie shape boundaries: tail fill, leaf push into the root, height growth.
Coverage of
kotlinx.collections.immutable.implementations.immutableListby the deterministic suites (stress tests excluded):