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

rescue exceptions and run job.fail in perform #88

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mcfiredrill
Copy link

I needed to test some of my resque failure code, but I realized that Resque::Job#fail is not actually run if you are using inline. I think using ResqueSpec.inline simplifies testing quite a bit, so I wanted to use it, but I still needed failures to be reported. So I wrote this patch. I'm not sure if this fits everyones use case, or if this is even the right way to go about it.

Let me know what you think.

@mcfiredrill
Copy link
Author

Oops, well there is definitely at least one problem. It seems that on_failure_hooks are running twice now.

@leshill
Copy link
Owner

leshill commented Nov 27, 2013

Hi @mcfiredrill,

An alternative approach would be to test fail directly, something like:

  describe "MyJob.fail" do
    it "updates the fail count" do\
      expect do
        MyJob.fail
      end.to change(MyJob, :fail_count)
    end
  end

@mcfiredrill
Copy link
Author

I changed my test slightly and fixed duplicate failure hook bug by upgrading resque. Let me know what you think, although the tests still aren't passing on travis for some reason.

@mcfiredrill
Copy link
Author

Ah ok, so I'm actually relying on redis running for that test, I didn't realize that. I'm not sure yet how to test it without relying on redis.

@leshill
Copy link
Owner

leshill commented Dec 2, 2013

Hi @mcfiredrill,

Have you overridden the fail code in resque? I suspect the default behavior is using redis. Perhaps you should mock out fail and set the expectation that way?

@mcfiredrill
Copy link
Author

Nah I'm not overriding it, I'm just calling job.fail directly, like worker.rb does here:
https://github.com/resque/resque/blob/1-x-stable/lib/resque/worker.rb#L235
https://github.com/resque/resque/blob/1-x-stable/lib/resque/job.rb#L291

Mocking out fail sounds like a good idea, I'll try that out.

@mcfiredrill
Copy link
Author

So I did that, let me know what you think. Maybe I'm wary of using any_instance but I'm not sure how to get the specific instance, maybe if enqueue returned the job instance...

@leshill
Copy link
Owner

leshill commented Dec 3, 2013

Hi @mcfiredrill,

Hmmm. Looking at that, it is not exactly what we want. Job#fail is what has the Redis dependency, and stubbing it in the specs still leaves the dependency at runtime.

We want a test double at runtime for Job#fail. If you want to try to write it, please do. If not I will be doing something like this in perform:

def perform(queue_name, payload)
  job = new_job(queue_name, payload)
  begin
    job.perform
  rescue Object => e
    fail_job(job, e)
  end
end

def fail_job(job, e)
  # run failure hooks here
end

Make sense?

@mcfiredrill
Copy link
Author

Alright, I guess I kind of missed the point there. I'm actually testing a custom failure backend via this, which I guess I shouldn't be doing, I should be doing an integration test with full resque. I can still set inline manually without resque_spec.

I'm still having fun hacking on this project though, so I think I'll attempt to write this.

@kyrylo
Copy link

kyrylo commented Nov 3, 2015

Any news/plans on this? At the moment I'm using a fork of this gem with this PR merged in.

@chrishough
Copy link

bump, I am tracking this too. I am trying to test job retries right now and can not seem to get around this :(

@kyrylo
Copy link

kyrylo commented Aug 28, 2016

@chrishough
Copy link

@kyrylo are you referring to this ::

When /the (\w+) queue runs/ do |queue_name|
  ResqueSpec.perform_all(queue_name)
end

in that gem? do also use Resque.inline = true or keep that false?

@kyrylo
Copy link

kyrylo commented Aug 29, 2016

It's just a fork where this PR is merged. You don't want to touch Resque.inline, if I recall correctly. Unfortunately, I don't test job retries, but it works quite well for testing normal jobs.

@leshill
Copy link
Owner

leshill commented Aug 29, 2016

Hi all,

Thanks for following up, this project needs a new maintainer (#122). Please drop a note if interested.

Regards.

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

Successfully merging this pull request may close these issues.

4 participants