-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
…t5`, we don't *have* to put all tests in undertest. But call detection *is* probably easier if you're not in `com.diffplug.selfie`.
…ple snapshots in one place.
- all build was failing bc of recordCall
selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/WriteTracker.kt
Outdated
Show resolved
Hide resolved
- as List<CallLocation>
@nedtwigg was expecting some tests to fail, like: |
@nedtwigg can we merge this? |
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() |
There was a problem hiding this comment.
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 [email protected]/java.lang.reflect.Method.invoke(Method.java:568)
at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1511)
at [email protected]/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
- Carriage return handling #16
- and then literal snapshots
class IntSelfie(private val actual: Int) { fun toBe(expected: Int): Int = TODO() fun toBe_TODO(): Int = TODO() } fun expectSelfie(actual: Int) = IntSelfie(actual)
Sorry, I had this review comment stuck as pending. Yes you can merge this if you want, but we'll need to revisit it to make sure we're doing assertions on the error messages somehow. I think most natural to do in this PR, but we could do that in a future PR too. |
- keep using `AssertionFailedError` - parse message and stacktrace (error are nice now)
@nedtwigg updated with nice error handling/stacktraces. |
Amazing! I love your solution. Next up:
|
Implementation of
Also the very beginnings of inline literal snapshots.
@jknack I'm hoping that you will write the whole literal snapshot system if you're interested in it. This PR is the first step of that feature. It's a good next step after #13 gets finished.