Skip to content

[screengrab] Fix screenshot copy count and path probe output#30038

Merged
iBotPeaches merged 1 commit into
fastlane:masterfrom
nakshatrasinghh:30037-fix-screengrab-exit-status-spam
May 21, 2026
Merged

[screengrab] Fix screenshot copy count and path probe output#30038
iBotPeaches merged 1 commit into
fastlane:masterfrom
nakshatrasinghh:30037-fix-screengrab-exit-status-spam

Conversation

@nakshatrasinghh

@nakshatrasinghh nakshatrasinghh commented May 17, 2026

Copy link
Copy Markdown
Contributor

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.
  • I've added or updated relevant unit tests.

Motivation and Context

Resolves #30037

screengrab checks several possible on-device screenshot directories while cleaning old screenshots and pulling newly captured screenshots. Missing directories are expected during this search, because only one of the candidate paths may exist for a given Android version, storage location, or app configuration.

Those expected misses were still routed through the generic command executor as normal command failures. As a result, every missing candidate path printed both the ls: ... No such file or directory output and Exit status: 1, causing very noisy logs across all locales.

While investigating the report, I also found that the final screenshot summary could say Captured 0 new screenshots! even when screenshots were copied. That happened when screenshots overwrote existing files: the previous implementation calculated the count as the difference between the destination file count before and after pulling.

Description

The investigation traced the noisy output to Screengrab::Runner#if_device_path_exists. That helper intentionally probes candidate screenshot paths using adb shell ls, and already treats missing paths as safe to ignore. The problem was that the command executor printed the failure output before the screengrab helper could ignore it.

This change adds an opt-in suppress_error_output parameter to FastlaneCore::CommandExecutor.execute. The command still returns output and status to the caller, and still raises when no error callback is supplied, but callers that expect a non-zero status can suppress replaying the command output and Exit status line.

screengrab now uses that option only for the expected missing-path probes. Other adb commands continue to use the existing behavior.

This also updates screenshot counting so screengrab counts the PNG files actually copied from the pulled screenshot directories. That makes the final summary accurate even when copied screenshots replace files that already existed in the output directory.

Unit coverage was added for:

  • suppressing expected command failure output while preserving status handling
  • preserving normal raise behavior when suppressed failures are not handled
  • missing screengrab device paths not yielding or printing expected adb failures
  • copied screenshot count being returned even when existing files are overwritten
  • copied screenshot count from pulled screenshot directories

Testing Steps

I reproduced the issue by following the path from the reported adb shell ls failures back to the screengrab directory probing helper. A missing path produced the same behavior shown in the issue: the missing-file message followed by Exit status: 1.

After the fix, the same missing-path probe still reports the non-zero status to the caller, but no longer prints the expected failure to the user.

I also reproduced the screenshot-count issue by simulating an existing output screenshot and a successful copy of a screenshot with the same destination count. Before the fix, the method reported 0; after the fix, it reports the number of screenshots copied.

@iBotPeaches iBotPeaches left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One comment. I like the changes to the counts as well, since this appears to hook the line of the actual copy vs the other counting device vs host to determine difference.

Comment thread fastlane_core/lib/fastlane_core/command_executor.rb Outdated
@nakshatrasinghh nakshatrasinghh force-pushed the 30037-fix-screengrab-exit-status-spam branch from ef2082e to 17f66b6 Compare May 18, 2026 17:03

@iBotPeaches iBotPeaches left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, last change and I think this will be good to go. I don't like how we effectively swallow output now, which will make this difficult to debug in future.

Lets hook suppress_error_output up to the verbose property. So if you opt-into VERBOSE you still get this output to help debugging.

@nakshatrasinghh

Copy link
Copy Markdown
Contributor Author

Okay, last change and I think this will be good to go. I don't like how we effectively swallow output now, which will make this difficult to debug in future.

Lets hook suppress_error_output up to the verbose property. So if you opt-into VERBOSE you still get this output to help debugging.

I feel the best fix for this is to make the CommandExecutor ignore suppress_error_output when verbose is enabled, so normal runs stay quiet but --verbose still shows the suppressed command output and exit status for debugging. I’ll add coverage for both the normal failure path and the PTY rescue path.

@nakshatrasinghh nakshatrasinghh force-pushed the 30037-fix-screengrab-exit-status-spam branch from 17f66b6 to dc89366 Compare May 19, 2026 15:50
@nakshatrasinghh

Copy link
Copy Markdown
Contributor Author

Pushed the update now. suppress_error_output is now ignored when verbose mode is enabled, so normal runs stay quiet but --verbose still shows the command output and exit status for debugging.

I also added specs for both the normal non-zero exit path and the PTY rescue path.

@iBotPeaches iBotPeaches merged commit a080451 into fastlane:master May 21, 2026
9 checks passed

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @nakshatrasinghh 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a month, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀"

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Congratulations! 🎉 This was released as part of fastlane 2.235.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fastlane screengrab console gets spammed with Exit status: 1

2 participants