Skip to content
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

Open
maltoe opened this issue Nov 16, 2018 · 3 comments
Open

Test suite is resistant to changes in Sidekiq #6

maltoe opened this issue Nov 16, 2018 · 3 comments

Comments

@maltoe
Copy link
Contributor

maltoe commented Nov 16, 2018

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 on sleep in the tests, instead of all this UnitOfWork 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?

@dsander
Copy link
Member

dsander commented Nov 19, 2018

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.

@maltoe
Copy link
Contributor Author

maltoe commented Nov 20, 2018

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 @boss stuff in test_helper.rb is only there to advance time without waiting.

Nevertheless, I'd like to get rid of this very aspect of it. Sidekiq has this fake! testing mode, maybe it's possible to rely on that instead of Redis, and maybe job execution "after time" can be simulated by simply executing the job?


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.

gem total DLs according to bestgems.org
sidekiq-repeat 5,018
sidekiq-repeating-jobs 1,824
sidekiq-scheduler 1,967,359
sidekiq-dejavu 6,688
sidetiq (with clock thread) 1,861,730
sidekiq-cron (with clock I think) 4,295,824

But is has over 2000 LoC 😄 And they call it "lightweight"...

@dsander
Copy link
Member

dsander commented Nov 20, 2018

Nevertheless, I'd like to get rid of this very aspect of it. Sidekiq has this fake! testing mode, maybe it's possible to rely on that instead of Redis, and maybe job execution "after time" can be simulated by simply executing the job?

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?

Side note: While looking into the code, I re-discovered sidekiq-scheduler.

Interesting to see their approach, having a configuration file avoids the need to require the jobs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants