[scan] fix regressions in unit tests

[wuaar1003]

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 #21728

Description

Return version 0 instead of nil for correct memoization, which also resolves the issue of passing nil to Gem::Version

Testing Steps

New unit test to exercise code path.

[rogerluan]

@wuaar1003 thanks for this follow-up PR! 🙏

It seems to me that, conceptually/semantically, a "nil" value would make more sense than a "Version of 0", no? Could we work around the issue observed while keep using nil? Of course, this would mean not attempting to instantiate a Gem::Version.new(nil) but instead guarding against that scenario from happening completely

[AliSoftware]

@wuaar1003 thanks for this follow-up PR! 🙏

It seems to me that, conceptually/semantically, a "nil" value would make more sense than a "Version of 0", no? Could we work around the issue observed while keep using nil? Of course, this would mean not attempting to instantiate a Gem::Version.new(nil) but instead guarding against that scenario from happening completely

Was reviewing this PR and was about to say the same before seeing Roger's comment already mentioning it 😄 I agree that still returning nil would feel more idiomatic to Ruby's standards, i.e. os_version.nil? ? nil : Gem::Version.new(os_version) or Gem::Version.new(os_version) unless os_version.nil? (tip: in Ruby, that last form automatically evaluates to nil if the if condition is false)

[lacostej]

@wuaar1003 I've made a PR to address the code review. wuaar1003#1

[rogerluan]

Thanks @lacostej I've reviewed your PR there :)

IMO you can merge that in here after that comment is addressed, so we can run the test suite and see if everything checks out

[wuaar1003]

Thanks for the helpful feedback and PR. It turns out that I did experiment with using nil version instead of 0 -- see this diff.

The reason why tests are timing out is because caching does not capture nil:

    def self.default_os_version(os_type)
      @os_versions ||= {}
      @os_versions[os_type] ||= begin
       ...
          gem_version = Gem::Version.new(os_version) if os_version
        end
        gem_version
      end
    end

Thus, we will be doing output parsing every time for OS types which do not have a default. IIRC, this was my rationale for using version 0 as a stand-in.

Apologies for not staying on top of this. I don't foresee having the bandwidth to track this actively so will close for now.