Remove integration of Delayed Job #70

Merged
merged 1 commit into from Jan 4, 2016

Projects

None yet

6 participants

@jeroenvandijk
Contributor

Currently email_spec is working of jobs when it has detected Delayed Job. I think this is really undesirable.

  • First it causes unexpected behavior. I've once spent some hours to detect why my scenario didn't seem to populate any jobs. After a while I figured that email-spec had already worked them off.
  • Secondly it is unnecessary. I spent a long time debugging when my test suite run time exploded after adding a particular scenario with a lot of objects being created. My test suites run time went from 3 minutes to 8 minutes. The reason was that all those object had after save callbacks with background jobs. This meant that there was a big queue of background jobs. For this particular scenario they didn't have to be run, but email-spec triggered the worker anyways causing a big extra delay.

So in summary I think the integration of delayed job should be removed. If it is necessary for your scenario to pass, it is better to explicitly call the worker yourself IMHO.

WDYT?

Jeroen

@jeroenvandijk
Contributor

I also wanted to add that I couldn't get the test suite to pass on my machine (before my change). And I couldn't find a test for the Delayed Job part.

@doolin
doolin commented Aug 4, 2011

I'm getting caught with this right now. +1 for removing delayed_job.

@danielmorrison

I like having it there (as a big user of DJ) but I can see why it would frustrate people.

What about making it a more generic hook and then you can just require 'email_spec/delayed_job' or write your own?

@bmabey
Collaborator
bmabey commented Apr 8, 2012

Sorry, catching up on pull requests and I just saw this. :/ I can understand why the current behaviour is frustrating. I like @danielmorrison suggestion of keeping the functionality in the gem but having it be opt-in. Are there any downsides to that approach?

@yfeldblum

Delayed::Job already provides a mechanism for this.

# initializer
Delayed::Worker.delay_jobs = !Rails.env.test?

# before filter
Delayed::Worker.stub(:delay_jobs) { false }

# just before asserting on emails
Delayed::Job.each(&:invoke_job)

The usage of queues should be explicit throughout the application, as well as throughout the tests. Using queues is good, but it must be explicit to retain that goodness. Hiding the use of queues is generally a bad idea, including in tests.

@etagwerker etagwerker self-assigned this Jan 3, 2016
@etagwerker
Collaborator

I agree with @jeroenvandijk and @doolin. I don't think it's necessary inside the email-spec gem. If we have DelayedJob in, then it means we should also have Sidekiq, Resque and any other background job gem.

This means that the gem will continue to grow in size for something that is not core to the goal of this library.

Also, it can produce unexpected behavior when you call last_email_sent as it works off DJ, without considering if there are other jobs in the queue. https://github.com/bmabey/email-spec/blob/master/lib/email_spec/background_processes.rb#L9

IMO programmers should handle their queues however they want to, without any interference by email-spec

Finally, I think that this method assumes too much and that these methods go against the principle of least astonishment

@etagwerker etagwerker merged commit 91e9b4c into email-spec:master Jan 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment