Remove ActiveSupport from being a primary dependency.

[pat]

Hi fine Bikkies 👋🏻

I just went to add the test collector to a Hanami app, and noticed it was bringing along ActiveSupport as a dependency. I don't have that gem within my app, have a self-imposed challenge to keep it that way, and thought I'd investigate whether it's actually required here. I don't think it is?

There are three uses I've found and adjusted (in separate commits below):

• The blank?/present? convenience methods - which yes, are convenient, but also easily changed to pure Ruby options.
• Hash#with_indifferent_access - as far as I could see within the code, the hashes are created with symbol keys and strings aren't being used to refer to the keys at later points. The tests are also fine with this being removed, so, I hope this is fine!
• ActiveSupport::Notifications - which is a harder thing to replace, but it turns out it's only being used for ActiveRecord tracing. In which case I feel it's safe to presume that if ActiveSupport isn't already loaded by the app's test suite, then I'm pretty sure ActiveRecord won't be in play.

With all of these changes sorted, I've then shifted ActiveSupport to be a development dependency. Tests are happy for me locally, and I've run this in the Hanami app's test suite and a Rails app's test suite, and both runs have appeared in the test analytics interface. If there's context I've missed though, please do let me know!

[niceking]

Hey @pat! Thanks for this PR! I'll test it out on Buildkite's test suite and run it by the team, and will get back to you about this!

[timriley]

Just noting we do have some failing tests here.

[pat]

Thanks @timriley :) Just with regards to the tests - they're green locally, but the CI results aren't public, so I can't see what's not working or why. Which is to say: I can't fix those up, sorry! 😅

[niceking]

The only other thing I looked for here was whether we had tests for the tracing functionality, so we could e.g. add another with ActiveSupport stubbed out to be undefined, just so we have coverage that ensures the gem works without it.

Yeah true there's no test around this, though with Pat's manual testing and this PR I feel pretty confident that the tracing hasn't broken. But yeah would be good to add a test to catch any future regressions! The other bit where traces are used is in the write to artifact file feature which is undocumented and AFAIK only we use it, and I've just checked that's still working too

[niceking]

Oh I didn't realise you couldn't see the build! I wonder if we should make this pipeline public 🤔

For your visibility @pat, the tests are failing on a legacy ruby build, ruby version 2.3 and this is the error log:

An error occurred while loading spec_helper.
--
  | Failure/Error: OpenSSL::SSL::SSLError,
  |  
  | NameError:
  | uninitialized constant Buildkite::TestCollector::Uploader::OpenSSL
  | # ./lib/buildkite/test_collector/uploader.rb:17:in `<class:Uploader>'
  | # ./lib/buildkite/test_collector/uploader.rb:4:in `<module:TestCollector>'
  | # ./lib/buildkite/test_collector/uploader.rb:3:in `<top (required)>'
  | # ./lib/buildkite/test_collector.rb:19:in `require_relative'
  | # ./lib/buildkite/test_collector.rb:19:in `<top (required)>'
  | # ./spec/spec_helper.rb:3:in `require'
  | # ./spec/spec_helper.rb:3:in `<top (required)>'
  | No examples found.
  | No examples found.
  |  
  |  
  | Finished in 0.00004 seconds (files took 0.09797 seconds to load)
  | 0 examples, 0 failures, 1 error occurred outside of examples
  |  
  | Finished in 0.00004 seconds (files took 0.09797 seconds to load)
  | 0 examples, 0 failures, 1 error occurred outside of examples

[pat]

Hmm, not sure whether it's ActiveSupport or something else that was requiring OpenSSL already, but I've put in a direct require given the references to the error classes. Hopefully that gets things green in MRI 2.3 🤞🏻

[pat]

And thanks for the failure details @niceking :)

[niceking]

Hey @pat your change has been merged and released, it's available in v2.5.0 :)

[pat]

Thank you! 💚