Ruby3 Upgrade

[xandroc]

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

• • A short explanation of the proposed change: Ruby3 upgrade with fix for scheduler errors

• An explanation of the use cases your change solves: Keep in sync with latest Ruby version and prepare for future features/upgrades

• Links to any other associated PRs:

• I have reviewed the contributing guide

• I have viewed, signed, and submitted the Contributor License Agreement

• I have made this pull request to the main branch

• I have run all the unit tests using bundle exec rake

• I have run CF Acceptance Tests

[linux-foundation-easycla]

• ✅ login: moleske / name: M. Oleske (9800110, ee48e2a)
• ✅ login: sethboyles / name: Seth Boyles (2c0b7e7)
• ❌ - login: @dalvarado / name: david . The commit (f17639d) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[sethboyles]

@xandroc @moleske any idea why our tests weren't able to catch this?

[sethboyles]

Probably because we mock out the DistributedScheduler here:

cloud_controller_ng/spec/unit/lib/cloud_controller/clock/clock_spec.rb

Line 111 in 2f2aa34

allow(DistributedScheduler).to receive(:new).and_return scheduler

😞

Of course now that keyword arguments are more strict the following expectation isn't entirely accurate:

cloud_controller_ng/spec/unit/lib/cloud_controller/clock/clock_spec.rb

Lines 113 to 119 in 2f2aa34

	expect(scheduler).to receive(:schedule_periodic_job).with(
	interval: interval,
	name: job_name,
	fudge: Clock::FREQUENT_FUDGE_FACTOR,
	thread: true,
	timeout: timeout,
	).and_yield

Because we know it wasn't receiving keyword arguments, it was receiving a hash. But the expectation didn't fail. I wonder if newer versions of rspec deal with the new keyword arguments better.

[moleske]

There's no unit test around clock.rb, so that's why are usual bundle exec rake didn't see anything. We had trouble finding a good unit test to add, but can take another look. Technically, the sync integration test caught, but was difficult to debug. It has made me appreciate the sync integration tests a bit more now that we had to look at them for the first time in a long while

edit: just saw your second comment. That makes sense. I'll try to look at what newer rspec does if anything. But wouldn't hold up this PR on newer rspec

[sethboyles]

Seems like we are on newest Rspec already.

[sethboyles]

This issue implies Rspec does make the distinction, so I'm not sure why it didn't trigger here: rspec/rspec-mocks#1438

[sethboyles]

might as well go to 3.0.4, since that has been released since we first attempted the upgrade:

https://www.ruby-lang.org/en/news/2022/04/12/ruby-3-0-4-released/

[moleske]

done, bumped it in capi-release pr

[sethboyles]

I pointed the rspec gems to the main branches and ran the clock spec, and it caught the error:

Randomized with seed 14706
F

Failures:

  1) VCAP::CloudController::Clock scheduling a daily job schedules with a 1 day interval at the given time with the given name and enqueues the job
     Failure/Error: DistributedScheduler.new.schedule_periodic_job(job_opts, &block)

       #<InstanceDouble(VCAP::CloudController::DistributedScheduler) (anonymous)> received :schedule_periodic_job with unexpected arguments
         expected: ({:at=>"12:00", :fudge=>1 minute, :interval=>1 day, :name=>"fake"}) (keyword arguments)
              got: ({:at=>"12:00", :fudge=>1 minute, :interval=>1 day, :name=>"fake"}) (options hash)
       Diff:

     # ./vendor/cache/rspec-support-528d88ce6fac/lib/rspec/support.rb:102:in `block in <module:Support>'
     # ./vendor/cache/rspec-support-528d88ce6fac/lib/rspec/support.rb:111:in `notify_failure'
     # ./vendor/cache/rspec-mocks-eb6d7e934743/lib/rspec/mocks/error_generator.rb:338:in `notify'
     # ./vendor/cache/rspec-mocks-eb6d7e934743/lib/rspec/mocks/error_generator.rb:322:in `__raise'
     # ./vendor/cache/rspec-mocks-eb6d7e934743/lib/rspec/mocks/error_generator.rb:55:in `raise_unexpected_message_args_error'
     # ./vendor/cache/rspec-mocks-eb6d7e934743/lib/rspec/mocks/message_expectation.rb:555:in `raise_unexpected_message_args_error'
     # ./vendor/cache/rspec-mocks-eb6d7e934743/lib/rspec/mocks/proxy.rb:222:in `message_received'
     # ./vendor/cache/rspec-mocks-eb6d7e934743/lib/rspec/mocks/method_double.rb:80:in `proxy_method_invoked'
     # ./vendor/cache/rspec-mocks-eb6d7e934743/lib/rspec/mocks/verifying_proxy.rb:161:in `proxy_method_invoked'
     # ./vendor/cache/rspec-mocks-eb6d7e934743/lib/rspec/mocks/method_double.rb:64:in `block (2 levels) in define_proxy_method'
     # ./lib/cloud_controller/clock/clock.rb:58:in `schedule_job'
     # ./lib/cloud_controller/clock/clock.rb:21:in `schedule_daily_job'
     # ./spec/unit/lib/cloud_controller/clock/clock_spec.rb:40:in `block (3 levels) in <module:CloudController>'
     # ./vendor/cache/rspec-core-814c1c1ecf79/lib/rspec/core/example.rb:263:in `instance_exec'
     # ./vendor/cache/rspec-core-814c1c1ecf79/lib/rspec/core/example.rb:263:in `block in run'

What a bummer. See this branch: https://github.com/cloudfoundry/cloud_controller_ng/compare/rspec-on-main?expand=1

Upsetting that this is not released yet, would have saved us a large headache. I am going to try to run this on the rest of our units to see if we missed any other spots.

[sethboyles]

The issue isn't the double, it's the expect().to receive().with().

[sethboyles]

@xandroc @moleske By pointing the rspec gems to main and removing conflicting dependencies indiscriminately, I was able to find one other potential issue:

 69) VCAP::Services::ServiceBrokers::V2::Client#deprovision makes a delete request with correct message
      Failure/Error: @http_client = VCAP::Services::ServiceBrokers::V2::HttpClient.new(http_client_attrs)

        #<VCAP::Services::ServiceBrokers::V2::HttpClient (class)> received :new with unexpected arguments
          expected: ({:auth_password=>"auth_password-1357", :auth_username=>"auth_username-1357", :url=>"https://foo.com/url-1632"}) (keyword arguments)
               got: ({:auth_password=>"auth_password-1357", :auth_username=>"auth_username-1357", :url=>"https://foo.com/url-1632"}) (options hash)
        Diff:

         Please stub a default value first if message might be received with other args as well.
      # ./lib/services/service_brokers/v2/client.rb:14:in `initialize'
      # ./spec/unit/lib/services/service_brokers/v2/client_spec.rb:15:in `new'
      # ./spec/unit/lib/services/service_brokers/v2/client_spec.rb:15:in `block (2 levels) in <module:V2>'
      # ./spec/unit/lib/services/service_brokers/v2/client_spec.rb:1936:in `block (3 levels) in <module:V2>'
      # ./spec/spec_helper.rb:179:in `block (4 levels) in <top (required)>'
      # ./spec/support/database_isolation.rb:37:in `cleanly'
      # ./spec/spec_helper.rb:179:in `block (3 levels) in <top (required)>'
      # /Users/pivotal/.gem/ruby/3.0.4/bin/bundle:23:in `load'
      # /Users/pivotal/.gem/ruby/3.0.4/bin/bundle:23:in `<main>'

It looks like it was just the test mocking that was mismatched, not the actual code, so not that alarming. But unfortunately I couldn't all specs to run since I had to remove a few dependencies that weren't compatible with rspec main branches. So it's possible there are a few remaining spots that got missed.

[philippthun]

/easycla

[philippthun]

Hmm, how can the EasyCLA check be re-run? Closing and re-opening the PR had no effect as well as commenting /easycla. Or is @dalvarado not authorized?

[moleske]

@dalvarado did sumbit it, cause I watched him do it. It seems to be stuck with VMware. If we're ready to merge and he's still not authorized we'll put his name second. EasyCLA seems to only check the first author

[FloThinksPi]

IMHO we could cut a ruby-3.0 exclusive release (meaning it will be the only change in the capi release so easy rollback can be achieved). Any objections and news on the CLA issue @moleske ? : )

[sethboyles]

@moleske and I managed to bundle CCNG with the latest references to the rspec gems and ran bake. The tests uncovered a few more places where the specs were not expecting keywords or hash opts correctly. No cases of the actual code being incorrect, however.

We did discover that it was still possible to get a false positive on a test, if the mocks overwrote the method expecting the keyword arguments/hash opts to match the way the offending caller miscalls the method. So unfortunately its not foolproof, but we have a little more confidence than before.

We think this is probably the most we can do before merging this PR in again. @moleske and I will cut a release today and then merge in this PR to prepare for a Ruby3 only release. We are going to merge despite the CLA warming since we have eye-witness testimony that @dalvarado did indeed sign the CLA.