-
Notifications
You must be signed in to change notification settings - Fork 216
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
Bump Truth 1.1.5 => 1.2.0 #1206
Conversation
assertThat(Build.DISPLAY).isEqualTo("sdk_phone_armv7-userdebug UpsideDownCakePrivacySandbox URD9.231106.004.A2 11164158 test-keys") | ||
assertThat(Build.ID).isEqualTo("URD9.231106.004.A2") |
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.
How did truth/guava affect these?
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.
good point, i think just non-hermetic update from Google's side. I can move into a separate PR.
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.
I'm happy either way, was just surprised.
Do I understand it correctly that this change depends on the Android SDK via compileSdk?
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.
Yes, but now this change is leading me to pull on a long thread haha. When I push an empty commit, CI passes, but if i bump Truth, this test fails due to an updated sdk preview....why? Is our Gradle caching setup on CI busted in some way?
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.
Very likely
- mandatory mention Explicitly declare inputs to the Test task #997
- but also if something randomly reads a file from the SDK during
test
that must be also declared as an input, otherwise Gradle doesn't know.
Based on "empty commit" mention, it sounds like the second, assuming that when you push empty commit no test gets re-executed (which should be the case if every task is cached by gradle-build-action).
How this relates to truth upgrade is beyond me still though. You can try modifying this file in a non-meaningful way (e.g. add a comment) and see if it fails or passes.
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.
Did a deep search of the SDK and found that this is located in: platforms\android-34\build.prop
, then noticed that in the Paparazzi codebase this is mentioned here:
paparazzi/paparazzi/src/main/java/app/cash/paparazzi/internal/Renderer.kt
Lines 112 to 124 in aec3fa3
val platformDataRoot = System.getProperty("paparazzi.platform.data.root") | |
?: throw RuntimeException("Missing system property for 'paparazzi.platform.data.root'") | |
val platformDataDir = File(platformDataRoot, "data") | |
val fontLocation = File(platformDataDir, "fonts") | |
val nativeLibLocation = File(platformDataDir, getNativeLibDir()) | |
val icuLocation = File(platformDataDir, "icu" + File.separator + "icudt70l.dat") | |
val keyboardLocation = File(platformDataDir, "keyboards" + File.separator + "Generic.kcm") | |
val buildProp = File(environment.platformDir, "build.prop") | |
val attrs = File(platformDataResDir, "values" + File.separator + "attrs.xml") | |
val systemProperties = DeviceConfig.loadProperties(buildProp) + mapOf( | |
// We want Choreographer.USE_FRAME_TIME to be false so it uses System_Delegate.nanoTime() | |
"debug.choreographer.frametime" to "false" | |
) |
So the fix for this is to add all of those files as inputs to the test task, otherwise Gradle thinks that nothing has changed and load from cache.
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.
nice find!
Ok, so PrepareResourcesTask will re-run when compileSdkVersion changes:
abstract val compileSdkVersion: Property<String>
which will cause the test task to re-run again
test.systemProperties["paparazzi.test.resources"] =
writeResourcesTask.flatMap { it.paparazziResources.asFile }.get().path
the issue here is 99% of the time $ANDROID_HOME/platforms/android-${compileSdkVersion}
will have static contents, but 1% (when using sandbox preview sdks), this will not be the case, as we're seeing here because build.prop is changed, but not being detected.
I'd hate to change the plugin to crawl through the entire platformDir checking for file content changes for the 1% test case, especially given initial findings that we might be caching huge payloads, so I think the "right fix" here is to disable caching for that specific plugin test.
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.
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.
Another option -- if this comes up again -- would be to force the PrepareResourcesTask to rerun when compileSdkVersion doesn't parse to an integer value.
add("testImplementation", "com.google.guava:guava") { | ||
attributes { | ||
attribute( | ||
TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE, | ||
objects.named(TargetJvmEnvironment, TargetJvmEnvironment.STANDARD_JVM) | ||
) | ||
} | ||
because("LayoutLib and sdk-common depend on Guava's -jre published variant." + | ||
"See https://github.com/cashapp/paparazzi/issues/906.") | ||
} |
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.
Could/should this be somehow done in a way that it affects all of Paparazzi's users? If someone uses Paparazzi on testImplementation
, they'll likely run into issues (we have so many reported in this repo).
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.
I can't think of a way, without force bundling Guava/Truth, which I'm reluctant to do. @JakeWharton, any thoughts?
32e1b49
to
4a717ff
Compare
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.
LGTM
Relates to #906. Closes #1195.
Guava's publishing changed slightly in v33.0.0, using Gradle Module Metadata to help guide consumers' resolution of the appropriate variant, i.e., -jre vs -android: google/guava#3683
Truth 1.2.0 bumps to Guava 33.0.0, so the preceding PR was failing because that new publishing mechanism by default forces the -android variant, which doesn't have the methods we expect and that are present in the -jre variant.
This is a frustrating ongoing problem and this PR is only solving the most recent manifestation of this issue. See the discussion in google/guava#6801 for the latest discussion.