Skip to content
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

Enable executing screenshot task without running instrumentation #52

Merged

Conversation

Laimiux
Copy link
Contributor

@Laimiux Laimiux commented Jan 12, 2019

📌 References

🎩 What is the goal?

This allows you to integrate remote service such as Firebase Test Lab with screenshot reporting.

How is it being implemented?

We provide a flag to disable instrumentation tests that run as part of screenshot task suite.

shot {
  runInstrumentationTests = false
}

User can now instead provide screenshot data in screenshots/screenshots-default folder and just call executeScreenshotTests.

How can it be tested?

  • Add fake screenshot data to shots-consumer/app/screenshots/screenshots-default
  • Disable the runInstrumentationTests flag
  • Run executeScreenshotTests


executeScreenshot.dependsOn(DownloadScreenshotsTask.name)
if (extension.runInstrumentation) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fully possible I misunderstand this here but should the executeScreenshot task not depend on the RemoveScreenshotsTask (line 70) task unless runInstrumentation is true?

Since we're not pulling the screenshots from a device when the instrumentation is not run, we're likely in an environment where neither a device nor adb is available (which is the case in the original issue).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, updated the PR.

executeScreenshot.dependsOn(RemoveScreenshotsTask.name)
val extension =
project.getExtensions.getByType[ShotExtension](classOf[ShotExtension])
if (extension.runInstrumentation) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cosmetically, this if-check and the next 2 vals could've gone inside the following if (extension.runInstrumentation), but generally looks good to me. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! I mostly wanted to get feedback on the approach, to make sure I'm going in the right direction.

@pedrovgs
Copy link
Owner

@Laimiux @Android-s14 thank you so much for your contribution. The PR code is just perfect. I'd add some documentation about the usage of this new parameter extension as part of the readme, but I'm going to do it by myself while preparing the next library release. Thank you so much for your contribution 👏

@pedrovgs pedrovgs merged commit a4d4376 into pedrovgs:master Feb 26, 2019
@pedrovgs
Copy link
Owner

I forgot to mention I didn't wait for the CI build to pass because we are using bitrise for this project and something changed on the configuration of the emulator and the build has been broken for a while. Sorry for the inconveniences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants