[screengrab] Fix screenshot copy count and path probe output#30038
Conversation
iBotPeaches
left a comment
There was a problem hiding this comment.
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.
ef2082e to
17f66b6
Compare
iBotPeaches
left a comment
There was a problem hiding this comment.
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 |
17f66b6 to
dc89366
Compare
|
Pushed the update now. I also added specs for both the normal non-zero exit path and the PTY rescue path. |
There was a problem hiding this comment.
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 🚀"
There was a problem hiding this comment.
Congratulations! 🎉 This was released as part of fastlane 2.235.0 🚀
Checklist
bundle exec rspecfrom the root directory to see all new and existing tests passbundle exec rubocop -ato ensure the code style is validci/circlecibuilds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)Motivation and Context
Resolves #30037
screengrabchecks 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 directoryoutput andExit 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 usingadb 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_outputparameter toFastlaneCore::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 andExit statusline.screengrabnow 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
screengrabcounts 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:
Testing Steps
I reproduced the issue by following the path from the reported
adb shell lsfailures back to the screengrab directory probing helper. A missing path produced the same behavior shown in the issue: the missing-file message followed byExit 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.