-
Notifications
You must be signed in to change notification settings - Fork 17
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
[POC] Switch to writing to additional test output and device provider #82
base: main
Are you sure you want to change the base?
Conversation
…s into rharter/device-provider
} | ||
.all { testTask -> |
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.
should be configureEach
to avoid task creation
else -> return@all | ||
} | ||
val defaultTestOutputDirectory = layout.buildDirectory | ||
.dir("outputs/androidTest-results/$deviceProviderName/${variant.name}") |
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 was having trouble figuring our additionalTestOutputDir
and I don't think it's actually used because of the return in testDataProvider
evaluation above.
I think collpasing additionalTestOutputEnabled
and additionalTestOutputDir
would be a lot more readable.
val additionalTestOutputDir = when (testTask) {
is DeviceProviderInstrumentTestTask -> testTask.additionalTestOutputEnabled.flatMap {
if(it) { testTask.additionalTestOutputDir } else { provider { null } }
}
is ManagedDeviceInstrumentationTestTask -> testTask.getAdditionalTestOutputEnabled().flatMap {
if(it) { testTask.getAdditionalTestOutputDir() } else { provider { null } }
}
is ManagedDeviceTestTask -> testTask.getAdditionalTestOutputEnabled().flatMap {
if(it) { testTask.getAdditionalTestOutputDir() } else { provider { null } }
}
else -> return@configureEach
}
I would also consider combining it with the testDataProvider
so that you only have a single when
to read, but you'd need a data class of a pair return type so it's a little more subjective.
public abstract class DeviceProviderFactory { | ||
@Suppress("DEPRECATION") | ||
@get:Internal | ||
internal var deviceProvider: DeviceProvider? = null |
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.
Is this assigned anywhere? What's the reason for using a Nested Bean instead of just some helper method for getDeviceProvider
?
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 is modeled after AGP, and I left this as a provider to enable tests that don't require connecting to actual devices. Those tests aren't implemented, and may not be, but I left this to leave that possibility.
This is a proof of concept implementation to explore the changes discussed in issue #31 (this comment specifically) to allow support for writing reference and diff images to the
additional_test_output
directory and leverage the AGP DeviceProvider to accessing devices.This is a fairly substantial change, so I'm creating a draft for feedback. There are several user-facing changes, most importantly the reference screenshot directory structure changes from
src/androidTest/screenshots
tosrc/androidTest/screenshots/connected
for the existing tests that are supported, andsrc/androidTest/screenshots/{device_name}
for any additional devices (not validated yet).So far this change is tested with connected tests, with the intention that this will support Gradle managed devices, but I'm still testing and validating that. I wanted to share this draft for feedback and testing before it gets too big.
There are still some unknowns with this approach. For instance,
DeviceProvider
is deprecated, but it's unclear what the accessible replacement is. I also don't love the way thatTestRunConfig
is written to the device and would prefer to be able to add instrumentation arguments to the text execution instead, for reading at runtime.