Add support for ActiveSupport classes in Sidekiq::Worker methods

[iMacTia]

Motivation

The currently tapioca-generated signatures for Sidekiq::Worker methods do not support ActiveSupport classes.
But reading the code comments and testing the behaviour at runtime, the methods should indeed be able to support them.

"+interval+ must be a timestamp, numeric or something that
acts numeric (like an activesupport time interval).

Implementation

ActiveSupport classes have been added to #perform_at and #perform_in methods, keeping their semantic difference despite these being aliased:

• Add ActiveSupport::TimeWithZone as an allowed type to #perform_at
• Add ActiveSupport::Duration as an allowed type to #perform_in

This is based on the assumption that projects using Sidekiq are most likely Rails application and therefore have access to ActiveSupport. If we don't want to rely on that assumption, the implementation will need to be tweaked to check if the ActiveSupport classes are defined when generating the signatures.

Tests

No new tests, just amended the existing tests to make sure the signatures are generated correctly

[KaanOzkan]

This is based on the assumption that projects using Sidekiq are most likely Rails application and therefore have access to ActiveSupport.

I don't think we can assume that. It doesn't depend on Rails and this will lead to type checking errors.

[amomchilov]

Hey @iMacTia, thanks for contributing this!

As Kaan mentioned, we can't assume that ActiveSupport and/or Rails are available. Luckily, this check already exists in ActiveSupportTimeExt. Could you please modify your PR to use that logic, and approach the appropriate time type?

tapioca/lib/tapioca/dsl/compilers/active_support_time_ext.rb

Lines 40 to 51 in 2dcdda3

	ConstantType = type_member { { fixed: T.class_of(::Time) } }
	sig { override.void }
	def decorate
	return unless constant.respond_to?(:zone)
	root.create_path(constant) do |mod|
	return_type = if ::Time.zone
	"::ActiveSupport::TimeWithZone"
	else
	"::Time"
	end

[iMacTia]

Thank you both for reviewing, I figured this might be an issue hence why I pointed it out in the description.
No problem, I'll add the conditional rendering and update the tests to test both 👍

[iMacTia]

@KaanOzkan @amomchilov I'm hitting a bit of a bump with testing both scenarios (ActiveSupport being bundled or not). Unfortunately I'm not as proficient with Minitest as I am with RSpec, so I may be missing something obvious here, but since we require "active_support" in the bin/test file I'm finding it hard to test what happens when `ActiveSupport doesn't exist.

I think I found a working solution in my latest commit, but I'm not very proud of the remove_const/const_set dance, and not sure if there's a more idiomatic way of achieving the same in Minitest?

If you have any suggestion I'm happy to make amendments or if it works and you don't mind we could just merge it as-is 🙂

[iMacTia]

All tests except for one (unrelated?) are currently passing on my branch 🎉
Let me know if there's anything else missing here, I'd love to get this merged (and released?) sooner rather than later as to avoid monkey-patching on our side 🙏

[KaanOzkan]

Another way is to use SpecWithProject similar to CLI tests. You'll be running it in a test gem so it should be a matter of requiring and not requiring ActiveSupport. But this way will be slower.

[iMacTia]

Soooo which approach do you think we should be using in this case?
I got it working with the current approach, but it's your gem so it's really up to you how you'd like this to be tested 🙂

[KaanOzkan]

I don't have a strong opinion, if after_teardown doesn't run somehow we probably have more important issues than not resetting ActiveSupport. I'll leave it to @amomchilov

[paracycle]

None of this is needed, you can include Tapioca::Helpers::Test::Isolation into this test class to make each of the tests run in a forked subprocess, each with its own clean copy of the object space.

[iMacTia]

Aaaah now we're talking! I'll give that a try, thank you @paracycle!

[iMacTia]

I also couldn't figure out how to run a single or a subset of tests, that would be quite useful because right now I can only run the whole test suite which takes quite some time 😞

Never mind, I finally found bin/test

[iMacTia]

OK it wasn't super straightforward because active_support is actually required in bin/test (apparently for Rails 6 support?) and so it's already part of the object space at the time of forking. That means I still need to undefine and unload it, but at least I don't need to put it back after the tests have run 👍

I grabbed some code from pipeline_spec.rb to do that and tests are passing locally 🙌