Skip to content

Commit a9f7f90

Browse files
nedtwiggnedtwigg
authored
Fix vcr (#525)
2 parents 630d0de + 35921b0 commit a9f7f90

File tree

8 files changed

+129
-72
lines changed

8 files changed

+129
-72
lines changed

‎jvm/CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
1111
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
1212

1313
## [Unreleased]
14+
### Fixed
15+
- Selfie VCR is now out of beta, no opt-in required. ([#525](https://github.com/diffplug/selfie/pull/525))
16+
- ArrayMap now sorts strings with multi-digit numbers as `1 2 ... 9 10 11` instead of `1 11 2 ...`.
17+
- Improved VCR-specific error messages for determining why `//selfieonce` might not be working for a test rule.
18+
- Fixed some bugs in VCR data storage (specifically concurrency and multiple frames).
1419

1520
## [2.5.0] - 2025-02-21
1621
### Added

‎jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/ArrayMap.kt

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2023-2024 DiffPlug
2+
* Copyright (C) 2023-2025 DiffPlug
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -23,16 +23,48 @@ private val STRING_SLASHFIRST =
2323
val charA = a[i]
2424
val charB = b[i]
2525
if (charA != charB) {
26-
return@Comparator ( // treat
27-
if (charA == '/') -1 // slash as
28-
else if (charB == '/') 1 // the lowest
29-
else charA.compareTo(charB) // character
30-
)
26+
// Check for slash first as it's special
27+
if (charA == '/') return@Comparator -1 // treat slash as the lowest character
28+
if (charB == '/') return@Comparator 1
29+
30+
// Check for embedded numbers
31+
if (charA.isDigit() && charB.isDigit()) {
32+
// Extract the complete numbers from both strings
33+
val numA = extractNumber(a, i)
34+
val numB = extractNumber(b, i)
35+
36+
// Compare the numeric values
37+
val numCompare = numA.first.compareTo(numB.first)
38+
if (numCompare != 0) return@Comparator numCompare
39+
40+
// If the numbers are equal, adjust index to after the numbers
41+
// and continue comparing the rest of the string
42+
i =
43+
maxOf(numA.second, numB.second) -
44+
1 // -1 because we increment i at the end of the loop
45+
} else {
46+
// Regular character comparison
47+
return@Comparator charA.compareTo(charB)
48+
}
3149
}
3250
i++
3351
}
3452
a.length.compareTo(b.length)
3553
}
54+
55+
/**
56+
* Extracts a numeric substring starting at the given index.
57+
*
58+
* @return Pair of (numeric value, index after the last digit)
59+
*/
60+
private fun extractNumber(s: String, startIndex: Int): Pair<Int, Int> {
61+
var endIndex = startIndex
62+
while (endIndex < s.length && s[endIndex].isDigit()) {
63+
endIndex++
64+
}
65+
val number = s.substring(startIndex, endIndex).toInt()
66+
return Pair(number, endIndex)
67+
}
3668
private val PAIR_STRING_SLASHFIRST =
3769
Comparator<Pair<String, Any>> { a, b -> STRING_SLASHFIRST.compare(a.first, b.first) }
3870

‎jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/Mode.kt

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,15 @@ enum class Mode {
4747
msg("Snapshot " + SnapshotNotEqualErrorMsg.forUnequalStrings(expected, actual))
4848
internal fun msgSnapshotMismatchBinary(expected: ByteArray, actual: ByteArray) =
4949
msgSnapshotMismatch(expected.toQuotedPrintable(), actual.toQuotedPrintable())
50-
internal fun msgVcrMismatch(key: String, expected: String, actual: String) =
51-
msg("VCR frame $key " + SnapshotNotEqualErrorMsg.forUnequalStrings(expected, actual))
52-
internal fun msgVcrUnread(expected: Int, actual: Int) =
53-
msg("VCR frames unread - only $actual were read out of $expected")
54-
internal fun msgVcrUnderflow(expected: Int) =
55-
msg(
56-
"VCR frames exhausted - only $expected are available but you tried to read ${expected + 1}")
50+
internal fun msgVcrSnapshotNotFound(call: CallStack) = msgVcr("VCR snapshot not found", call)
51+
internal fun msgVcrMismatch(key: String, expected: String, actual: String, call: CallStack) =
52+
msgVcr("VCR frame $key " + SnapshotNotEqualErrorMsg.forUnequalStrings(expected, actual), call)
53+
internal fun msgVcrUnread(expected: Int, actual: Int, call: CallStack) =
54+
msgVcr("VCR frames unread - only $actual were read out of $expected", call)
55+
internal fun msgVcrUnderflow(expected: Int, call: CallStack) =
56+
msgVcr(
57+
"VCR frames exhausted - only $expected are available but you tried to read ${expected + 1}",
58+
call)
5759
private fun ByteArray.toQuotedPrintable(): String {
5860
val sb = StringBuilder()
5961
for (byte in this) {
@@ -70,11 +72,19 @@ enum class Mode {
7072
when (this) {
7173
interactive ->
7274
"$headline\n" +
73-
(if (headline.startsWith("Snapshot "))
74-
"‣ update this snapshot by adding `_TODO` to the function name\n"
75-
else "") +
75+
"‣ update this snapshot by adding `_TODO` to the function name\n" +
7676
"‣ update all snapshots in this file by adding `//selfieonce` or `//SELFIEWRITE`"
7777
readonly -> headline
7878
overwrite -> "$headline\n(didn't expect this to ever happen in overwrite mode)"
7979
}
80+
private fun msgVcr(headline: String, call: CallStack) =
81+
when (this) {
82+
interactive ->
83+
"$headline\n" +
84+
"‣ update all snapshots in this file by adding `//selfieonce` or `//SELFIEWRITE`\n" +
85+
"‣ could not find control comment in ${call.location.ideLink(Selfie.system.layout)}\n" +
86+
"‣ remember to call `Selfie.vcrTestLocator()` in the test itself, or put the file above into the `selfie` package to mark that it is not the test file"
87+
readonly -> headline
88+
overwrite -> "$headline\n(didn't expect this to ever happen in overwrite mode)"
89+
}
8090
}

‎jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/Selfie.kt

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -94,14 +94,5 @@ object Selfie {
9494
* control whether the VCR is writing or reading. If the caller lives in a package called
9595
* `selfie.*` it will keep looking up the stack trace until a caller is not inside `selfie.*`.
9696
*/
97-
@JvmStatic
98-
@ExperimentalSelfieVcr
99-
fun vcrTestLocator(sub: String = "") = VcrSelfie.TestLocator(sub, deferredDiskStorage)
97+
@JvmStatic fun vcrTestLocator(sub: String = "") = VcrSelfie.TestLocator(sub, deferredDiskStorage)
10098
}
101-
102-
@RequiresOptIn(
103-
level = RequiresOptIn.Level.WARNING,
104-
message = "This API is in beta and may change in the future.")
105-
@Retention(AnnotationRetention.BINARY)
106-
@Target(AnnotationTarget.CLASS, AnnotationTarget.FUNCTION, AnnotationTarget.PROPERTY)
107-
annotation class ExperimentalSelfieVcr

‎jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/VcrSelfie.kt

Lines changed: 57 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,10 @@ package com.diffplug.selfie
1717

1818
import com.diffplug.selfie.guts.CallStack
1919
import com.diffplug.selfie.guts.DiskStorage
20+
import com.diffplug.selfie.guts.atomic
2021
import com.diffplug.selfie.guts.recordCall
22+
import com.diffplug.selfie.guts.reentrantLock
23+
import com.diffplug.selfie.guts.withLock
2124

2225
private const val OPEN = "«"
2326
private const val CLOSE = "»"
@@ -28,73 +31,98 @@ internal constructor(
2831
private val call: CallStack,
2932
private val disk: DiskStorage,
3033
) : AutoCloseable {
34+
/** Creates VCRs who store their data based on where this TestLocator was created. */
3135
class TestLocator internal constructor(private val sub: String, private val disk: DiskStorage) {
3236
private val call = recordCall(false)
3337
fun createVcr() = VcrSelfie(sub, call, disk)
3438
}
39+
private val state: State
40+
41+
internal sealed class State {
42+
class Read(val frames: List<Pair<String, SnapshotValue>>) : State() {
43+
fun currentFrameThenAdvance(): Int = cf.getAndUpdate { it + 1 }
44+
fun framesReadSoFar(): Int = cf.get()
45+
private val cf = atomic(0)
46+
}
3547

36-
private class State(val readMode: Boolean) {
37-
var currentFrame = 0
38-
val frames = mutableListOf<Pair<String, SnapshotValue>>()
48+
class Write : State() {
49+
private val lock = reentrantLock()
50+
private var frames: MutableList<Map.Entry<String, SnapshotValue>>? =
51+
mutableListOf<Map.Entry<String, SnapshotValue>>()
52+
fun add(key: String, value: SnapshotValue) {
53+
lock.withLock {
54+
frames?.apply {
55+
val idx = size + 1
56+
add(entry("$OPEN$idx$CLOSE$key", value))
57+
} ?: throw IllegalStateException("This VCR was already closed.")
58+
}
59+
}
60+
fun closeAndGetSnapshot(): Snapshot =
61+
Snapshot.ofEntries(
62+
lock.withLock {
63+
val frozen = frames ?: throw IllegalStateException("This VCR was already closed.")
64+
frames = null
65+
frozen
66+
})
67+
}
3968
}
40-
private val state: State
4169

4270
init {
4371
val canWrite = Selfie.system.mode.canWrite(isTodo = false, call, Selfie.system)
44-
state = State(readMode = !canWrite)
45-
if (state.readMode) {
72+
if (canWrite) {
73+
state = State.Write()
74+
} else {
4675
val snapshot =
4776
disk.readDisk(sub, call)
48-
?: throw Selfie.system.fs.assertFailed(Selfie.system.mode.msgSnapshotNotFound())
77+
?: throw Selfie.system.fs.assertFailed(
78+
Selfie.system.mode.msgVcrSnapshotNotFound(call))
4979
var idx = 1
80+
val frames = mutableListOf<Pair<String, SnapshotValue>>()
5081
for ((key, value) in snapshot.facets) {
5182
check(key.startsWith(OPEN))
5283
val nextClose = key.indexOf(CLOSE)
5384
check(nextClose != -1)
5485
val num = key.substring(OPEN.length, nextClose).toInt()
55-
check(num == idx)
86+
check(num == idx) { "expected $idx in $key" }
5687
++idx
5788
val keyAfterNum = key.substring(nextClose + 1)
58-
state.frames.add(keyAfterNum to value)
89+
frames.add(keyAfterNum to value)
5990
}
91+
state = State.Read(frames)
6092
}
6193
}
6294
override fun close() {
63-
if (state.readMode) {
64-
if (state.frames.size != state.currentFrame) {
95+
if (state is State.Read) {
96+
if (state.frames.size != state.framesReadSoFar()) {
6597
throw Selfie.system.fs.assertFailed(
66-
Selfie.system.mode.msgVcrUnread(state.frames.size, state.currentFrame))
98+
Selfie.system.mode.msgVcrUnread(state.frames.size, state.framesReadSoFar(), call))
6799
}
68100
} else {
69-
var snapshot = Snapshot.of("")
70-
var idx = 1
71-
for ((key, value) in state.frames) {
72-
snapshot = snapshot.plusFacet("$OPEN$idx$CLOSE$key", value)
73-
}
74-
disk.writeDisk(snapshot, sub, call)
101+
disk.writeDisk((state as State.Write).closeAndGetSnapshot(), sub, call)
75102
}
76103
}
77-
private fun nextFrameValue(key: String): SnapshotValue {
104+
private fun nextFrameValue(state: State.Read, key: String): SnapshotValue {
78105
val mode = Selfie.system.mode
79106
val fs = Selfie.system.fs
80-
if (state.frames.size <= state.currentFrame) {
81-
throw fs.assertFailed(mode.msgVcrUnderflow(state.frames.size))
107+
val currentFrame = state.currentFrameThenAdvance()
108+
if (state.frames.size <= currentFrame) {
109+
throw fs.assertFailed(mode.msgVcrUnderflow(state.frames.size, call), call)
82110
}
83-
val expected = state.frames[state.currentFrame++]
111+
val expected = state.frames[currentFrame]
84112
if (expected.first != key) {
85113
throw fs.assertFailed(
86-
mode.msgVcrMismatch("$sub[$OPEN${state.currentFrame}$CLOSE]", expected.first, key),
114+
mode.msgVcrMismatch("$sub[$OPEN${currentFrame}$CLOSE]", expected.first, key, call),
87115
expected.first,
88116
key)
89117
}
90118
return expected.second
91119
}
92120
fun <V> nextFrame(key: String, roundtripValue: Roundtrip<V, String>, value: Cacheable<V>): V {
93-
if (state.readMode) {
94-
return roundtripValue.parse(nextFrameValue(key).valueString())
121+
if (state is State.Read) {
122+
return roundtripValue.parse(nextFrameValue(state, key).valueString())
95123
} else {
96124
val value = value.get()
97-
state.frames.add(key to SnapshotValue.of(roundtripValue.serialize(value)))
125+
(state as State.Write).add(key, SnapshotValue.of(roundtripValue.serialize(value)))
98126
return value
99127
}
100128
}
@@ -107,11 +135,11 @@ internal constructor(
107135
roundtripValue: Roundtrip<V, ByteArray>,
108136
value: Cacheable<V>
109137
): V {
110-
if (state.readMode) {
111-
return roundtripValue.parse(nextFrameValue(key).valueBinary())
138+
if (state is State.Read) {
139+
return roundtripValue.parse(nextFrameValue(state, key).valueBinary())
112140
} else {
113141
val value = value.get()
114-
state.frames.add(key to SnapshotValue.of(roundtripValue.serialize(value)))
142+
(state as State.Write).add(key, SnapshotValue.of(roundtripValue.serialize(value)))
115143
return value
116144
}
117145
}

‎jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/guts/AtomicFuBroken.kt

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2024 DiffPlug
2+
* Copyright (C) 2024-2025 DiffPlug
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -24,12 +24,8 @@ expect class AtomicRef<T> {
2424
/** Replace with atomicfu when stable. */
2525
expect fun <T> atomic(initial: T): AtomicRef<T>
2626

27-
expect fun reentrantLock(): ReentrantLock
27+
expect class ReentrantLock
2828

29-
expect class ReentrantLock {
30-
fun lock(): Unit
31-
fun tryLock(): Boolean
32-
fun unlock(): Unit
33-
}
29+
expect fun reentrantLock(): ReentrantLock
3430

3531
expect inline fun <T> ReentrantLock.withLock(block: () -> T): T

‎jvm/selfie-lib/src/jsMain/kotlin/com/diffplug/selfie/guts/AtomicFuBroken.js.kt

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2024 DiffPlug
2+
* Copyright (C) 2024-2025 DiffPlug
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -29,12 +29,7 @@ actual class AtomicRef<T>(private var value: T) {
2929
}
3030
}
3131
val Lock = ReentrantLock()
32-
actual inline fun reentrantLock() = Lock
32+
actual fun reentrantLock() = Lock
3333

34-
@Suppress("NOTHING_TO_INLINE")
35-
actual class ReentrantLock {
36-
actual inline fun lock(): Unit {}
37-
actual inline fun tryLock() = true
38-
actual inline fun unlock(): Unit {}
39-
}
34+
@Suppress("NOTHING_TO_INLINE") actual class ReentrantLock
4035
actual inline fun <T> ReentrantLock.withLock(block: () -> T) = block()

‎jvm/selfie-lib/src/jvmMain/kotlin/com/diffplug/selfie/guts/AtomicFuBroken.jvm.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2024 DiffPlug
2+
* Copyright (C) 2024-2025 DiffPlug
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -24,7 +24,7 @@ actual class AtomicRef<T>(value: T) {
2424
actual fun updateAndGet(update: (T) -> T): T = ref.updateAndGet(update)
2525
actual fun getAndUpdate(update: (T) -> T) = ref.getAndUpdate(update)
2626
}
27-
actual inline fun reentrantLock() = ReentrantLock()
27+
actual fun reentrantLock() = ReentrantLock()
2828

2929
actual typealias ReentrantLock = java.util.concurrent.locks.ReentrantLock
3030
actual inline fun <T> ReentrantLock.withLock(block: () -> T): T {

0 commit comments

Comments
 (0)