takahirom / roborazzi

Make JVM Android integration test visible 🤖📸
https://takahirom.github.io/roborazzi/
Apache License 2.0
651 stars 24 forks source link

Fix output image consistency issue #366

Closed takahirom closed 1 month ago

takahirom commented 1 month ago

It seems that the task information provided by TestTaskSkipEventsServiceProvider, which is necessary to decide whether we should restore the cache to the output directory, isn't saved correctly. Therefore, I removed TestTaskSkipEventsServiceProvider and now always restore the cache. The performance impact appears to be minimal, and consistency is more important.

takahirom commented 1 month ago

The reason why the test fails is that when the test fails, testTask.doLast{} is not executed, so intermediateDir is not updated.

  @Test
  fun verifyAndRecord_changeDetect() {
    RoborazziGradleProject(testProjectDir).apply {
      record()
      val recordFileHash1 = getFileHash("$screenshotAndName.testCapture.png")
      changeScreen()

      verifyAndRecordAndFail()

      val recordFileHash2 = getFileHash("$screenshotAndName.testCapture.png")
      assert(recordFileHash1 != recordFileHash2)
takahirom commented 1 month ago

@lukas-mercari I think it is better to merge this fix, but we aren't able to reproduce the problem in our repository. Could you take a look?

lukas-mercari commented 1 month ago

@takahirom I believe the requirement to reproduce this is to make sure the task is taken from both configuration and build cache. (I can reproduce in the same way that I reproduced #374.)

Build cache: Because it means that the tests will be skipped. Configuration cache: In this case the skip detection will not work properly because expectingTestNames is not initialised correctly.

lukas-mercari commented 1 month ago

@takahirom I upgraded to the latest version in our CI/CD pipeline, but it seems that sometimes we get the following exception:

...
Caused by: java.io.IOException: Source file wasn't copied completely, length of destination file differs.
    at kotlin.io.FilesKt__UtilsKt.copyRecursively(Utils.kt:329)
    at kotlin.io.FilesKt__UtilsKt.copyRecursively$default(Utils.kt:288)
    at io.github.takahirom.roborazzi.RoborazziPlugin$apply$configureRoborazziTasks$6$8.afterSuite(RoborazziPlugin.kt:366)
    at jdk.internal.reflect.GeneratedMethodAccessor1126.invoke(Unknown Source)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
    ... 44 more

My hypothesis is that it might happen that while one suite finished (and afterSuite is called), another is still generating images -- and thus files in the intermediate dir keep changing.

takahirom commented 1 month ago

👀

takahirom commented 1 month ago

@lukas-mercari Thanks. I saw a similar issue. Are you running a single variant? https://github.com/takahirom/roborazzi/issues/377#issuecomment-2130691611

lukas-mercari commented 1 month ago

@takahirom Yes, we only run a single variant per runner. (E.g. ./gradlew recordRoborazziDebug)

takahirom commented 1 month ago

@lukas-mercari If your hypothesis is correct, we might have to check if it is the root suite. Are you seeing multiple 'Roborazzi: test.doLast Copy files' logs? https://stackoverflow.com/a/68518970

takahirom commented 1 month ago

In any case, we might need to include this if statement. https://github.com/search?q=%22suite.parent+%3D%3D+null%22&ref=opensearch&type=code

takahirom commented 1 month ago

@lukas-mercari Could you please take a look at this PR? https://github.com/takahirom/roborazzi/pull/385/files

lukas-mercari commented 1 month ago

Are you seeing multiple 'Roborazzi: test.doLast Copy files' logs?

Yes, I can see multiple logs (when running recording :sample-android:recordRoborazziDebug). Confirmed that the approach to find the root test suite seems to work!