Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track double writes (prepare for literal snapshots). #15

Merged
merged 9 commits into from
Sep 9, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,6 @@ class SnapshotFile {

var wasSetAtTestTime: Boolean = false
fun setAtTestTime(key: String, snapshot: Snapshot) {
// TODO: track whenever a snapshot is set, so that we can:
// - warn about duplicate snapshots when they are equal
// - give good errors when they are not
val newSnapshots = snapshots.plusOrReplace(key, snapshot)
if (newSnapshots !== snapshots) {
snapshots = newSnapshots
Expand Down
55 changes: 30 additions & 25 deletions selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/RW.kt
Original file line number Diff line number Diff line change
Expand Up @@ -21,34 +21,39 @@ package com.diffplug.selfie
* - if environment variable or system property named `ci` or `CI` with value `true` or `TRUE`
* - then Selfie is read-only and errors out on a snapshot mismatch
* - if environment variable or system property named `selfie` or `SELFIE`
* - its value should be either `read` or `write` (case-insensitive)
* - that will override the presence of `CI`
* - its value should be either `read`, `write`, or `writeonce` (case-insensitive)
* - `write` allows a single snapshot to be set multiple times within a test, so long as it is
* the same value. `writeonce` errors as soon as a snapshot is set twice even to the same
* value.
* - selfie, if set, will override the presence of `CI`
*/
internal object RW {
private fun lowercaseFromEnvOrSys(key: String): String? {
val env = System.getenv(key)?.lowercase()
if (!env.isNullOrEmpty()) {
return env
}
val system = System.getProperty(key)?.lowercase()
if (!system.isNullOrEmpty()) {
return system
internal enum class RW {
read,
write,
writeonce;

companion object {
private fun lowercaseFromEnvOrSys(key: String): String? {
val env = System.getenv(key)?.lowercase()
if (!env.isNullOrEmpty()) {
return env
}
val system = System.getProperty(key)?.lowercase()
if (!system.isNullOrEmpty()) {
return system
}
return null
}
return null
}
private fun calcIsWrite(): Boolean {
val override = lowercaseFromEnvOrSys("selfie") ?: lowercaseFromEnvOrSys("SELFIE")
if (override != null) {
return when (override) {
"read" -> false
"write" -> true
else ->
throw IllegalArgumentException(
"Expected 'selfie' to be 'read' or 'write', but was '$override'")
private fun calcRW(): RW {
val override = lowercaseFromEnvOrSys("selfie") ?: lowercaseFromEnvOrSys("SELFIE")
if (override != null) {
return RW.valueOf(override)
}
val ci = lowercaseFromEnvOrSys("ci") ?: lowercaseFromEnvOrSys("CI")
return if (ci == "true") read else write
}
val ci = lowercaseFromEnvOrSys("ci") ?: lowercaseFromEnvOrSys("CI")
return ci != "true"
val mode = calcRW()
val isWrite = mode != read
val isWriteOnce = mode == writeonce
}
val isWrite = calcIsWrite()
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ internal class ClassProgress(val className: String) {

private var file: SnapshotFile? = null
private var methods = ArrayMap.empty<String, MethodSnapshotGC>()
private var diskWriteTracker: DiskWriteTracker? = DiskWriteTracker()
// the methods below called by the TestExecutionListener on its runtime thread
@Synchronized fun startMethod(method: String) {
assertNotTerminated()
Expand Down Expand Up @@ -132,6 +133,7 @@ internal class ClassProgress(val className: String) {
}
// now that we are done, allow our contents to be GC'ed
methods = TERMINATED
diskWriteTracker = null
file = null
}
// the methods below are called from the test thread for I/O on snapshots
Expand All @@ -145,8 +147,10 @@ internal class ClassProgress(val className: String) {
}
@Synchronized fun write(method: String, suffix: String, snapshot: Snapshot) {
assertNotTerminated()
val key = "$method$suffix"
diskWriteTracker!!.record(key, snapshot, recordCall())
methods[method]!!.keepSuffix(suffix)
read().setAtTestTime("$method$suffix", snapshot)
read().setAtTestTime(key, snapshot)
}
@Synchronized fun read(
method: String,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* Copyright (C) 2023 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.diffplug.selfie.junit5

import com.diffplug.selfie.RW
import com.diffplug.selfie.Snapshot
import java.util.stream.Collectors

/** Represents the line at which user code called into Selfie. */
data class CallLocation(val subpath: String, val line: Int) : Comparable<CallLocation> {
override fun compareTo(other: CallLocation): Int {
val subpathCompare = subpath.compareTo(other.subpath)
return if (subpathCompare != 0) subpathCompare else line.compareTo(other.line)
}
override fun toString(): String = "$subpath:$line"
}
/** Represents the callstack above a given CallLocation. */
class CallStack(val location: CallLocation, val restOfStack: List<CallLocation>)
/** Generates a CallLocation and the CallStack behind it. */
fun recordCall(): CallStack {
val calls =
StackWalker.getInstance().walk { frames ->
frames
.skip(1)
.map { CallLocation(it.className.replace('.', '/') + ".kt", it.lineNumber) }
.collect(Collectors.toList())
}
return CallStack(calls.removeAt(0), calls)
}
/** The first write at a given spot. */
internal class FirstWrite<T>(val snapshot: T, val callStack: CallStack)

/** For tracking the writes of disk snapshots literals. */
internal open class WriteTracker<K : Comparable<K>, V> {
val writes = mutableMapOf<K, FirstWrite<V>>()
protected fun recordInternal(key: K, snapshot: V, call: CallStack) {
val existing = writes.putIfAbsent(key, FirstWrite(snapshot, call))
if (existing != null) {
if (existing.snapshot != snapshot) {
throw org.opentest4j.AssertionFailedError(
"Snapshot was set to multiple values:\nfirst time:${existing.callStack}\n\nthis time:${call}",
existing.snapshot,
snapshot)
} else if (RW.isWriteOnce) {
throw org.opentest4j.AssertionFailedError(
"Snapshot was set to the same value multiple times.", existing.callStack, call)
}
}
}
}

internal class DiskWriteTracker : WriteTracker<String, Snapshot>() {
fun record(key: String, snapshot: Snapshot, call: CallStack) {
recordInternal(key, snapshot, call)
}
}

class LiteralValue {
// TODO: String, Int, Long, Boolean, etc
}

internal class InlineWriteTracker : WriteTracker<CallLocation, LiteralValue>() {
fun record(call: CallStack, snapshot: LiteralValue) {
recordInternal(call.location, snapshot, call)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* Copyright (C) 2023 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.diffplug.selfie.junit5

import io.kotest.matchers.shouldBe
import kotlin.test.Test
import org.junit.jupiter.api.MethodOrderer
import org.junit.jupiter.api.Order
import org.junit.jupiter.api.TestMethodOrder
import org.junitpioneer.jupiter.DisableIfTestFails

/** Simplest test for verifying read/write of a snapshot. */
@TestMethodOrder(MethodOrderer.OrderAnnotation::class)
@DisableIfTestFails
class DuplicateWriteTest : Harness("undertest-junit5") {
@Test @Order(1)
fun noSelfie() {
ut_snapshot().deleteIfExists()
ut_snapshot().assertDoesNotExist()
}

@Test @Order(2)
fun cannot_write_multiple_things_to_one_snapshot() {
ut_mirror().linesFrom("fun shouldFail()").toFirst("}").uncomment()
ut_mirror().linesFrom("fun shouldPass()").toFirst("}").commentOut()
gradlew("underTest", "-Pselfie=write")?.printStackTrace()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is expected to fail, and in fact it is failing with:

org.opentest4j.AssertionFailedError: Snapshot was set to multiple values:
first time:com.diffplug.selfie.junit5.CallStack@6d5620ce

this time:com.diffplug.selfie.junit5.CallStack@311bf055
	at app//com.diffplug.selfie.junit5.WriteTracker.recordInternal(WriteTracker.kt:53)
	at app//com.diffplug.selfie.junit5.DiskWriteTracker.record(WriteTracker.kt:67)
	at app//com.diffplug.selfie.junit5.ClassProgress.write(SelfieTestExecutionListener.kt:151)
	at app//com.diffplug.selfie.junit5.Router.readOrWriteOrKeep(SelfieTestExecutionListener.kt:49)
	at app//com.diffplug.selfie.DiskSelfie.toMatchDisk(Selfie.kt:37)
	at app//com.diffplug.selfie.DiskSelfie.toMatchDisk$default(Selfie.kt:36)
	at app//undertest.junit5.UT_DuplicateWriteTest.shouldFail(UT_DuplicateWriteTest.kt:9)
	at java.base@17.0.4.1/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base@17.0.4.1/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base@17.0.4.1/java.util.ArrayList.forEach(ArrayList.java:1511)

We should make assertions about the error message (e.g. the current error message is not great).

Same thing is true of the writeonce_mode test below. It is supposed to fail, and it is, but we are not making any assertions about what the failure should be, which means it's hard for us to have good failure messages.

I'm not sure what the best way to make these assertions is. It's easy to catch an exception in the UT_ build and make the assertions there. The alternative is to parse the JUnit test result and pull the assertions into the "puppetmaster" build. No opinion which way to go, just that the message should have an assertion against it.

Once we are making assertions on the error message somehow, this PR is ready to merge, and you can move on to

}

@Test @Order(3)
fun can_write_one_thing_multiple_times_to_one_snapshot() {
ut_mirror().linesFrom("fun shouldFail()").toFirst("}").commentOut()
ut_mirror().linesFrom("fun shouldPass()").toFirst("}").uncomment()
gradlew("underTest", "-Pselfie=write") shouldBe null
}

@Test @Order(4)
fun can_read_one_thing_multiple_times_from_one_snapshot() {
ut_mirror().linesFrom("fun shouldFail()").toFirst("}").commentOut()
ut_mirror().linesFrom("fun shouldPass()").toFirst("}").uncomment()
gradlew("underTest", "-Pselfie=read") shouldBe null
}

@Test @Order(5)
fun writeonce_mode() {
ut_mirror().linesFrom("fun shouldFail()").toFirst("}").commentOut()
ut_mirror().linesFrom("fun shouldPass()").toFirst("}").uncomment()
gradlew("underTest", "-Pselfie=writeonce")?.printStackTrace()
}

@Test @Order(6)
fun deleteSelfie() {
ut_snapshot().deleteIfExists()
}
}
30 changes: 30 additions & 0 deletions selfie-runner-junit5/src/test/kotlin/testpkg/RecordCallTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright (C) 2023 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package testpkg

import com.diffplug.selfie.junit5.recordCall
import io.kotest.matchers.ints.shouldBeGreaterThan
import io.kotest.matchers.shouldBe
import org.junit.jupiter.api.Test

class RecordCallTest {
@Test
fun testRecordCall() {
val stack = recordCall()
stack.location.toString() shouldBe "testpkg/RecordCallTest.kt:26"
stack.restOfStack.size shouldBeGreaterThan 0
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package undertest.junit5

import com.diffplug.selfie.expectSelfie
import org.junit.jupiter.api.Test

class UT_DuplicateWriteTest {
// @Test fun shouldFail() {
// expectSelfie("apples").toMatchDisk()
// expectSelfie("oranges").toMatchDisk()
// }
@Test fun shouldPass() {
expectSelfie("twins").toMatchDisk()
expectSelfie("twins").toMatchDisk()
}
}