245: Add support for com.android.test modules - #333
DanielJette wants to merge 3 commits into
Conversation
|
Filed #339 for the IDE-side support I left out of this PR — gutter icons and |
AndroidTestifyBot
left a comment
There was a problem hiding this comment.
Changes requested
The plugin now applies to a com.android.test module, and the three plugin changes are individually correct. But the round trip the description presents as proof only works if the app under test is already on the device, and when it isn't, the task reports success.
1. screenshotTest does not install the target app, and passes anyway
On an API 37 emulator, with dev.testify.samples.flix and dev.testify.samples.flix.test uninstalled first:
$ ./gradlew FlixTest:screenshotTest
> Task :FlixTest:installDebug
Installing APK 'FlixTest-debug.apk' ...
INSTRUMENTATION_STATUS: Error=Unable to find instrumentation target package: dev.testify.samples.flix
INSTRUMENTATION_STATUS_CODE: -1
BUILD SUCCESSFUL in 11s
Two defects:
- Nothing installs
:FlixSample.testifySettingsshowsinstallTask = installDebug,installAndroidTestTask = null— thatinstallDebugis the test module's own APK. The app named bytargetProjectPathis never a dependency. - Zero tests ran and the build is green.
finalizeTaskActionlooks forFAILURES!!!,INSTRUMENTATION_CODE: 0andProcess crashed; this output matches none.FlixTest:screenshotRecordon the same device also reportsBUILD SUCCESSFUL, pulls nothing and records nothing.
The verification in the description passed because FlixSample was left installed from an earlier run. A user following the sample on a clean device or CI agent gets a silent false pass. The plugin can read TestExtension.targetProjectPath, so it can depend on that project's install task; and an instrumentation error status should fail the task.
2. Nothing in CI runs it
FlixTest isn't in bitrise.yml, and flix_sample.yml only assembles and tests :FlixSample. The committed baseline is never checked, so "a sample module that proves the round trip" is unguarded from the first commit. Running it in CI on a fresh emulator would also have caught (1).
3. The trap is documented only in a build-file comment
The description calls the applicationPackageId mix-up "a trap anyone writing a test module will hit". The mitigation is a comment in one sample's build.gradle. There is no docs page, no mention in settings.md or the library-projects recipe, and the plugin still infers nothing for a test module — both ids must be set by hand or validate() throws. The target id is derivable from targetProjectPath. At minimum this needs a recipe; better, inference.
4. Sample build file
- Unused dependencies copied from FlixSample:
hilt-navigation-compose,hilt-android,ktor-client-android,kotlinx-serialization-json,coil-compose,idling-concurrent. The single test renders aText. buildscript { ext { versions … } }andcomposeOptions { kotlinCompilerExtensionVersion }are dead with the Compose compiler plugin applied..gitignoregainsPlugins/IntelliJ/.kotlin/, which is unrelated and already covered by the existing.kotlin/line.
Verified
Plugin:ktlintCheck Plugin:test Plugin:assemblepass locally.baselineSourceDirresolves tosrc/main/assetsforFlixTest, and the pull destination follows it.- With FlixSample installed, the committed baseline matches on a
37-1080x2220@440dp-en_USemulator. - Merged with #332,
FlixTeststill configures.settings.gradlemerges cleanly with #334; CHANGELOG conflicts with the other five open code PRs.
|
Agreed on all four. Point 1 was the important one, and my verification had the hole you identified — FlixSample was installed from an earlier run, so I never saw the failure. 1. Both defects fixedThe target application is now installed. The plugin reads With both packages uninstalled first, which is your test: An instrumentation error now fails the task. This took a second attempt. My first fix added Worth flagging that 2. CI
3. Inference rather than a comment — agreed, and doneYou were right that the target id is derivable, so documenting a trap was the wrong answer. Both ids are now inferred and the sample configures neither:
There is also a Test-only modules section in 4. Sample build file
|
AndroidTestifyBot
left a comment
There was a problem hiding this comment.
Changes requested
Big improvement: target install, id inference, the docs section and CI coverage are all in, and I verified each. The failure detection still misses a case it is meant to catch, and the sample's test package collides with FlixSample's.
Verified
- Clean device: with
dev.testify.samples.flixanddev.testify.samples.flix.testuninstalled,FlixTest:screenshotTestruns:FlixSample:installDebugthen:FlixTest:installDebug, and givesOK (1 test)against the committed baseline. - Inference:
testifySettingsgivestargetPackageId = dev.testify.samples.flix,testPackageId = dev.testify.samples.flix.test,baselineSourceDir = src/main/assets, with neither id configured. runProcessswap (Runtime.exec→ProcessBuilder, which every adb call goes through):LegacySample:screenshotTestgivesOK (88 tests)andFlixSample:screenshotTestgivesOK (22 tests)on this branch. So the redLegacy Samplecheck doesn't reproduce here either; please re-run it.Plugin:ktlintCheck Plugin:test Plugin:assemblepass; docs build is clean; the sample build file is trimmed as asked.
1. "Target not installed" can still pass
I repeated your negative test: target uninstalled, install dependency bypassed.
$ adb uninstall dev.testify.samples.flix
$ ./gradlew FlixTest:screenshotTest -x FlixSample:installDebug
> Task :FlixTest:screenshotTest
android.util.AndroidException: INSTRUMENTATION_FAILED: dev.testify.samples.flix.test/androidx.test.runner.AndroidJUnitRunner
at com.android.commands.am.Instrument.run(Instrument.java:549)
BUILD SUCCESSFUL in 2s
Same emulator (API 37), same situation, but a different shape from the INSTRUMENTATION_STATUS: Error=Unable to find instrumentation target package I got in the first round. finalizeTaskAction only matches the second form, so zero tests ran and the build is green again.
Matching on error strings will keep missing variants. The robust check is the other way round: am instrument -w always ends a run that actually executed with OK (n tests) or FAILURES!!!, so treat a log with neither as a failure. Keep the explicit matches as well if you want the clearer message.
2. FlixTest's package is FlixSample's test package
testPackageId falls back to the namespace, dev.testify.samples.flix.test. That is also FlixSample's own androidTest APK id (applicationId "dev.testify.samples.flix" plus the default .test suffix). The two modules install over each other: after this run the device has a single dev.testify.samples.flix.test, and which APK it is depends on which task ran last. Both Bitrise steps in test_flix and anyone running FlixSample's tests from Android Studio after FlixTest are affected. Please give the sample a distinct namespace, e.g. dev.testify.samples.flix.screenshots.
3. The namespace isn't always the test APK's id
testModulePackageId reads TestExtension.namespace, but a com.android.test module can set defaultConfig.applicationId (and a build-type suffix), and that is what gets installed. Prefer defaultConfig.applicationId ?: namespace. The docs sentence "testPackageId is this module's own namespace" needs the same qualification.
|
Points 1 and 2 are fixed. Point 3 I dug into and I think it does not hold on AGP 9 — evidence below, happy to be shown otherwise. 1. Detection inverted, as you suggestedYou were right that matching error strings would keep missing variants, and your Your negative test: I also applied it to One thing worth noting in favour of 2. Namespace collision — confirmed and fixed
I left the Kotlin package as 3. AGP 9 ignores
|
AndroidTestifyBot
left a comment
There was a problem hiding this comment.
Approved
Both fixes hold and the applicationId finding is convincing.
Verified on an API 37 emulator, all Flix packages uninstalled first
$ ./gradlew FlixTest:screenshotTest > :FlixSample:installDebug, :FlixTest:installDebug ... OK (1 test)
$ ./gradlew FlixSample:screenshotTest OK (22 tests)
$ ./gradlew FlixTest:screenshotTest OK (1 test) # back to back, nothing clobbered
$ adb shell pm list packages | grep flix
dev.testify.samples.flix
dev.testify.samples.flix.screenshots
dev.testify.samples.flix.test
...
Negative cases, target uninstalled and its install bypassed:
$ ./gradlew FlixTest:screenshotTest -x FlixSample:installDebug THE SCREENSHOT TESTS DID NOT RUN!!! BUILD FAILED
$ ./gradlew FlixTest:screenshotRecord -x FlixSample:installDebug THE SCREENSHOT TESTS DID NOT RUN!!! BUILD FAILED
Extending the check to InternalScreenshotTestRecordTask closes the record half of the original finding, which I hadn't asked for explicitly.
No regressions from the detection change
LegacySample:screenshotTest:OK (88 tests).- A filtered
LegacySample:screenshotRecord -PtestClass=…:OK (2 tests), passes. Plugin:ktlintCheck Plugin:test Plugin:assemblepass. The docs build is clean. CI is fully green, including Legacy.
On point 3
Accepted. "The typed DSL doesn't expose it, the Groovy DSL silently ignores it, and the installed package and output-metadata.json both show the namespace" is a better standard of evidence than my claim. Recording that in the KDoc, and steering the docs away from <app id>.test, is the right outcome.
Keeping the Kotlin package as dev.testify.samples.flix.test is fine.
Merge note: #332 and this PR both edit ScreenshotTestTask.setDependencies. Whichever lands second needs a careful rebase, and the conflict won't only be in the CHANGELOG.
What does this change accomplish?
Fixes #245
Applying the plugin to a
com.android.testmodule failed at configuration time:Project.androidlooked only forApplicationExtensionandLibraryExtension. A test-only modulehas a
TestExtension, so the lookup fell through to theGradleException.How have you achieved it?
Three plugin changes, plus a sample module that proves the round trip.
Project.androidalso resolvesTestExtension, and a newProject.isTestModulereports it.baselineSourceDirdefaults to themainsource set for a test module. Acom.android.testmodule has no
androidTestsource set — its tests are itsmainsources — so the old default ofsrc/androidTest/assetspointed at a directory that does not exist.implementationrather thanandroidTestImplementation, for thesame reason.
Samples/Flix/FlixTestis a worked example: acom.android.testmodule withtargetProjectPath ':FlixSample'and one Compose screenshot test, with its recorded baseline.The two defects review found, both fixed here:
installDebuginstalls itstest APK; the application named by
targetProjectPathwas never a dependency, so on a cleandevice
am instrumentcould not find its target package.screenshotTestandscreenshotRecordnow depend on the target project's install task.
finalizeTaskActionlooks forFAILURES!!!,INSTRUMENTATION_CODE: 0andProcess crashed;Error=Unable to find instrumentation target packagematched none of them, so the task passed having run nothing. It now also fails onINSTRUMENTATION_STATUS: Error=— which required givingrunProcessan opt-inredirectErrorStream, because it read only standard output and the error never reached the log.The wider problem, that adb's exit code is discarded everywhere, is filed as The Gradle plugin discards adb's exit code and standard error #340.
Both package ids are inferred, so the sample configures neither.
applicationPackageIdcomesfrom the target project;
testPackageIdfrom the test module's namespace, since it has noapplicationIdof its own. Getting this wrong is a trap worth removing rather than documenting: withboth set to the test module's id,
screenshotRecordreported✓ Recording baselineandscreenshotPullthen looked in the wrong package and silently pulled nothing.Scope note. The branch this came from also carried IntelliJ plugin changes for gutter icons in
test modules. That work is unfinished — commented-out alternatives, duplicate imports and unbalanced
braces — and it does not compile, so it is not included here. #245 is specifically about the Gradle
plugin refusing to apply, which this closes. The IDE-side support is #339; it needs
runIdetoverify rather than CI.
The
FlixTestbuild file also needed modernising for the current toolchain: the standaloneorg.jetbrains.kotlin.androidplugin is rejected under AGP 9,lintOptionsis gone, and the Javatarget was still 21.
Scope of Impact and Testing instructions
Additive for application and library modules.
baselineSourceDirand the package-id inference changeonly when the module is a
com.android.test; verified unchanged forFlixSample,FlixLibrary,GmdSampleandLegacySample. The instrumentation-error check applies to every module, and is theone behaviour change that could turn a previously green build red — correctly, since such a build ran
no tests.
CHANGELOG entries added under Unreleased.
recipes/21-library-projects.mdhas a new Test-onlymodules section.
Verified locally:
End to end on an API 37 emulator, with both packages uninstalled first:
And the silent-pass check, with the install dependency disabled and the target removed:
FlixTestis in the Bitrisetest_flixworkflow, with APK paths checked against the build output.flix_sample.ymlonly assembles:FlixSample, so it does not cover this module.