-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test suite is resistant to changes in Sidekiq #6
Comments
It's been a long time since I read the code and the tests, to be honest I don't know why all the mocking is required. Just glancing over it it looks to me that we could test of lot of the code with unit tests and have a few integration tests that use the public sidekiq API. |
I looked into it again yesterday a bit, and found out that it's really not that stupid after all :) In fact, it's actually only little mocking involved (i.e. it really hits Redis, it really uses Sidekiq's dispatcher mechanism + middleware stack, it really uses Sidekiq's bootup mechanism for the initial loading of repeatable tasks). This whole processor and Nevertheless, I'd like to get rid of this very aspect of it. Sidekiq has this Side note: While looking into the code, I re-discovered sidekiq-scheduler. This gem has come a looong way since we started with sidekiq-repeat. It still has the same clockless scheduling approach, but has developed into a whole suite of sidekiq plugins and plugin plugins and so on.
But is has over 2000 LoC 😄 And they call it "lightweight"... |
Hmm do we even need to test the future execution? If we properly schedule the job to be run it should be safe to assume that Sidekiq does it's job and actually runs the job wen it is supposed to do, isn't it?
Interesting to see their approach, having a configuration file avoids the need to require the jobs. |
Big maybe. I remember we did all of this mocking and Sidekiq driving for a reason, but it seems awfully complicated now. And resistant to change in Sidekiq itself.
One idea would be to introduce a 6th cron star alongside a
secondly
method, and simply rely onsleep
in the tests, instead of all thisUnitOfWork
and processor stuff. Obviously not "proper" way to do it, but I'd argue that in a test suite so small, it wouldn't hurt if it took 10 seconds instead of 1.What do you think?
The text was updated successfully, but these errors were encountered: