spec_helper refactoring and fixing Scan tests

[dotdoom]

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

• some methods (like with_env_values) are defined in more than one spec_helper.rb, with the same signatures and functionality
• they also have different behavior. For example, the method below does not accept any block (contrary to what the comment says), which means that the block passed in is ignored (sic!), and any test code inside the block does not get executed. See DO NOT MERGE: Demo that some tests never run #14502 for a demo.

fastlane/fastlane_core/spec/spec_helper.rb

Lines 36 to 43 in 875e348

	# Executes the provided block after adjusting the ENV to have the
	# provided keys and values set as defined in hash. After the block
	# completes, restores the ENV to its previous state.
	def with_env_values(hash)
	hash.each do |k, v|
	stub_const('ENV', ENV.to_h.merge(k => v))
	end
	end

• stubbing.rb does not seem to add any value, so I simply moved its content into spec_helper.rb to have less entities. If the team feels strongly about having it as a separate file, I will delete that commit from this PR
• from several implementations of with_... methods (some more permissive than others) I picked / reimplemented with the following principles:
  • always take a block (this is a common convention for with_... methods in rspec. Ruby developers would expect it to take a block). Despite stub_const looks very pretty to use, it does not allow to limit scope (the scope always equals that of the whole it block) and requires a different naming convention (ex.: stub_env_values)
  • complain if the method is called without block (dropped unused feature to "permanently change environment variable" because it's not useful - ENV['MYVAR'] does the same).

[dotdoom]

Uh-oh, I've no idea what happened on AppVeyor. Time to dust off my virtualbox and download some Windows images.

[dotdoom]

The tests failing are those which were not running before the fix. See #14502 which "breaks" them but they keep passing.. Looks like they are severely out of date and can be removed. WDYT?

[janpio]

Ugh, I had looked at that with_env_values a bit ago and wondered how it works - but assumed I must be missing some magic here. How did you discover this?

Left some comments.

A guess regarding the two failing tests: Maybe the Slack part of scan was rewritten to use the Slack action and thus somehow messed things up - undetected by those tests that were never executed?

[dotdoom]

Hey, glad this sorts things out! I was actually fixing a Windows-specific issue in documentation generator and while writing a test, I needed to temporarily change environment.

I think the tests were written for Slack::Notifier, which no longer exists, and instead has SlackAction. Here's the PR that would break those tests, should they have run by that time: #13791.

[janpio]

Oh, pretty recent that test even. Well, I think nobody will miss those tests then. Bye 👋

[dotdoom]

After sitting for a while staring at the code over the weekend, I decided that it's worth fixing these tests. PTAL?

[janpio]

Yep, tests look right to me (although the last one threw me a bit, but as it passes this seems to be an appropriate way to ensure the call is doing what it should).

And: TIL PTAL ;)

[dotdoom]

Yes, I think this reaction on the test is appropriate. I had to use something I've never tried before (includes_hash) but as far as I can tell it's better than verifying all parameters, making the test a change detector (I think it's the right term?).

Haha, please pardon me this uncalled for "l33t sp3ak" outbreak, it is too addictive.

[dotdoom]

Is there anything else I can do for this PR?

[janpio]

No, I think we are just waiting for someone with more ruby chops to come along from the team and leave an "Approve" review. I don't fully trust myself here.

[dotdoom]

Hey there friendly Fastlane team, may I humbly request some feedback for this PR? It's blocking some of the further contributions I have in queue.

[janpio]

Look good from my side, but needs some more eyes from someone with more ruby.

(Pinged the appropriate people in the background @dotdoom)

[joshdholtz]

This is soooooo 💯 Thanks for taking the time to clean up our tests ❤️ ❤️ ❤️

[fastlane-bot]

Hey @dotdoom 👋

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 🚀

[fastlane-bot]

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