[screengrab] Fix escaping issue with adb path (#15981)

[Jeehut]

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've read the Contribution Guidelines
• I've updated the documentation if necessary.

Motivation and Context

Fixes #15981.

Description

I've removed the escaping of the adb tool path as that added a \ in front of the path which caused the issue. There's no need for shellescaping from my understanding of the examples and documentation pages for paths, especially since this path doesn't include any whitespaces.

Testing Steps

Just make sure fastlane screengrab is still working on different platforms. I've tested macOS only.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[Jeehut]

@googlebot I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[Jeehut]

Just checked the failing tests on Circle CI and I don't think they are related to the changes I made in this PR. It was probably failing because of the timing of the tests: The tests started at the end of the 6th of February but the check was done at the beginning of the 7th of February. So, just re-running the tests will fix the CI (can't do it myself though).

Update: Just reported the failing tests as a separate issue in #15987.

Update 2: I just force-pushed to force Circle CI to re-run the tests. Should pass now!

[janpio]

especially since this path doesn't include any whitespaces.

How do you know that? adb can be installed in whatever location the user chooses, so the path could have a space in it, right? .shellescape is used to make this unknown string safe for usage in a shell.

I think the problem is somewhere in the handling of an env var: #15981 (comment)

[janpio]

Don't think this is the actual problem or a good solution.

[Jeehut]

@janpio Good point, I've updated the solution to expand the adb_path instead, to make sure we don't run into problems with paths starting with ~. Can confirm that this solved the issue as well.

[janpio]

Ok, my feedback made sense in theory but in practice tests are not failing as they expect the unexpanded paths. Does that make sense? Should we update the tests? Or could this have any side effects? Should not, right?

[Jeehut]

@janpio I don't think it will have any side effects, but tests will become more cumbersome and less readable since we need to change things like expect(result).to eq("./fastlane/README.md test") to be something like expect(result).to eq("#{File.expand_path('./fastlane/README.md')} test") as we can't know the path on different devices statically. That's why I personally prefer the solution from the previous commit, especially given that paths starting with . are not affected by the shellescape issue in the first place, which I just tested in irb:

I'm reverting the last commit then, tell me if you rather prefer to update the tests instead.

But to be honest, I don't understand why expect(result).to eq("./fastlane/README.md test") is influenced by a change to the adb_path in the first place since the README file should have nothing to do with the adb_path, or am I missing something here?

[janpio]

That is just a bad example filename for a test that uses adb:

fastlane/fastlane/spec/actions_specs/adb_spec.rb

Lines 4 to 10 in 39afb4d

	it "generates a valid command" do
	result = Fastlane::FastFile.new.parse("lane :test do
	adb(command: 'test', adb_path: './fastlane/README.md')
	end").runner.execute(:test)
	expect(result).to eq("./fastlane/README.md test")
	end

[Jeehut]

I see, thanks for the explanation.

What is your opinion on the code of this PR? If you don't mind, we just merge it as is for the reasons explained above. It fixes the issue with the ~ and doesn't affect anything else. Checks have all passed.

[janpio]

I think we should undo the last commit, and then just call File.expand manually in the tests. Otherwise we have a special case in the code that we sure will forget about in a few weeks.

[Jeehut]

@janpio Okay, reverted back to the solution where we expand the adb_path at all times and made the relevant changes to the tests instead.

[janpio]

And we uncovered a problem on WIndows with that:

  1) Fastlane Fastlane::FastFile adb picks up path from ANDROID_HOME environment variable and handles path that has to be made safe
     Failure/Error: expect(result).to eq("#{File.expand_path(path)} test")
       expected: "C:/projects/fastlane/\"/usr/local/android-sdk/with space/platform-tools/adb\" test"
            got: "\"C:/usr/local/android-sdk/with space/platform-tools/adb\" test"
       (compared using ==)
     # ./fastlane/spec/actions_specs/adb_spec.rb:52:in `block (4 levels) in <top (required)>'

[janpio]

Wonderful and beautiful. Let's see if @joshdholtz agrees.

[joshdholtz]

Looks good to me! Thanks for looking into this ❤️ Really appreciate the contribution!

[fastlane-bot]

Hey @Jeehut 👋

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 week, 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 🚀