should not garbage collect snapshots when all tests are package-private

[iaincd]

Hello! Excited to start using this project.

I have a test for some generated HTML that is currently using a hard-coded fixture (barely tolerable with text blocks), and converting it with Selfie.expectSelfie(output).toMatchDisk_TODO() was a breeze.

I am having an issue with snapshot persistence however, wherein a subsequent Gradle invocation removes the artifact, and thereafter the test fails unless I add //SELFIEWRITE (or revert to the _TODO() invocation).

I'll try to add more details later (environment, etc.), but for now, the useful error breadcrumb I see is:

java.lang.AssertionError: Selfie wrote a snapshot and then marked it stale for deletion it in the same run: <path snipped>
Selfie will delete this snapshot on the next run, which is bad! Why is Selfie marking this snapshot as stale?
    at com.diffplug.selfie.junit5.Progress.finishedAllTests(SelfieTestExecutionListener.kt:316)

Let me know if there's anything else besides environment info I can add later that might help track this down. I can take a swing at an MVCE if necessary too. Thanks!

[nedtwigg]

It's likely that the test is being run in a way that our garbage collection doesn't recognize. Do any of the following describe your test? And if not, how is the test method annotated?

selfie/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieGC.kt

Lines 25 to 27 in 350dce5

	"org.junit.jupiter.api.Test", // junit5,
	"org.junit.jupiter.params.ParameterizedTest",
	"org.junit.Test" // junit4

[iaincd]

The first one (JUnit Jupiter).

Scanning the javadocs there, it sounds like maybe my snapshot is not being named correctly — it gets named like EntireTestClass.ss, i.e. not having any specificity to the test method from which Selfie is invoked. Is that wholly unexpected?

I do have the Mockito JUnit extension in the mix also, I can see if removing that has any effect.

[nedtwigg]

EntireTestClass.ss is expected, there is a 1:1 between test class and test snapshot file.

class NormalTestClass {
  @org.junit.jupiter.api.Test
  public void myTest() {
    expectSelfie("blah").toMatchDisk()

That's the most normal test class I can imagine. In what ways is yours different? Inner class? Custom test annotation?

[iaincd]

My problem seems to arise here:

selfie/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieGC.kt

Lines 53 to 61 in 350dce5

	private fun classExistsAndHasTests(key: String): Boolean {
	return try {
	Class.forName(key).methods.any { method ->
	testAnnotations.any { method.isAnnotationPresent(it) }
	}
	} catch (e: ClassNotFoundException) {
	false
	}
	}

Class.forName(key) seems to yield only the ~8 base methods of Object.class, and thus has no chance of finding my regular old @org.junit.jupiter.api.Test-annotated methods.

key looks to correctly reflect the test class name. 🤔

Could it be the case that this only works for public test methods? It is idiomatic in JUnit 5 for test methods to be package-private.

[iaincd]

I think that must be it — changing my test method signature from

@Test
void foo() {...}

...to:

@Test
public void foo() {...}

...no longer results in my snapshot being GC'd.

Do you think it would take much to handle package-private test methods? Common linters complain these days if JUnit 5 tests have unnecessary visibility.

[nedtwigg]

Amazing, thanks! A fix is on the way...

[nedtwigg]

Interesting... I added a reproducer in the PR above but it is working for public/package/protected.

What JRE are you on, and are you using a module-info.java file? I wonder if there's a module thing going on...

[iaincd]

JDK: Coretto 17.0.7 (2023-04-18 LTS)
Kotlin: 1.9.0
Gradle: 7.4.2

No module-info.java in the project. The jvm-test-suite Gradle plugin is in use, which might be relevant somehow, although it doesn't seem to be a matter of failing to locate the test sources.

Happy to try changing any of those versions on my end if it will help shed any light!

[nedtwigg]

Fixed in 1.1.1, just published, might take a minute to get through the caching layers.

The reproducer wasn't working because as long as there was at least one public method then it worked. I got fixated on the modulepath thing, but that was a red herring.

Here's the commit where the reproducer fails, fixing it was straightforward from there: 8e59325

Thanks for reporting!

[iaincd]

You’re welcome, thank you for shipping a fix so quickly!