brandonhilkert / sucker_punch

Sucker Punch is a Ruby asynchronous processing library using concurrent-ruby, heavily influenced by Sidekiq and girl_friday.
MIT License
2.64k stars 114 forks source link

Testing the worker object #15

Closed kitop closed 11 years ago

kitop commented 11 years ago

I'm having some troubles when testing the woker object with ActiveRecord. It throws an ActiveRecord::RecordNotFound error.

Here's the simplified code: https://gist.github.com/kitop/5242297

I also have database_cleaner set up on the Rspec.config.

There's no calling async code I believe, which is what I first suspected that could be the problem.

Any idea? What's the ideal way to test the workers?

brandonhilkert commented 11 years ago

All workers are just standard classes, so you should be able to test them like you would any straight forward Ruby code. Just a quick glance looks like:

user = user.find(user_id)

should be:

user = User.find(user_id)
kitop commented 11 years ago

Sorry, that was a typo when simplifying the example.

That's what I thought. That it was a standard class. But when I remove the include SuckerPunch::Worker statement (and remove the initializer config) tests pass without trouble.

I'm only requiring 'sucker_punch/testing' on my spec/spec_helper.rb. Should I need to require something else?

brandonhilkert commented 11 years ago

including sucker_punch/testing just allows you to stub out calls to the Queue class and interogate how many jobs are in there. But testing the worker class should have nothing to do with the queue or the other options sucker_punch/testing/inline. That would be for more integration style tests, to check that the job got called from a controller invokation or actual browser run, and have it done inline so the assertions won't get confused.

What are the errors you're getting? Can you post the Rails app or a sample for me to see?

kitop commented 11 years ago

Brandon, here's a striped down app that I get the spec to fail: https://github.com/kitop/sucker_punch_error_example

It has only one test, but it has a full setup of testing environment, as I have in the original app just in case there's something causing the error there.

Thanks for your help!

brandonhilkert commented 11 years ago

Took a quick look and confirmed the behavior you saw. Doing some more poking later. Will be in touch.

kitop commented 11 years ago

Great thanks!!

I tried changing the DatabaseCleaner set up in spec/spec_helper.rb from :transaction to :truncation and tests passed as expected.

So I'm guessing that the perform method could be running in a separate thread anyway? And database gets cleaned up before.

For now I can change the cleanup method for the workers specs only and it works. But would be good to know why that's happening.

brandonhilkert commented 11 years ago

I did the same thing and found that it fixed it, but like you mentioned, it seems odd. I have an email out to the Celluloid list which is the underlying tech that makes those jobs run async. Hope to know more soon.

brandonhilkert commented 11 years ago

Turns out the class is pushed into a new thread on instantiation, rather than during .async, so therefore, the transaction is occurring in the thread outside of the work as you mentioned. Unfortunately it seems like this is the way it is. I'll have to update the README. Thanks for uncovering this oddity.

brandonhilkert commented 11 years ago

Mind sharing your spec_helper config to fix it so I can add it to the README?

kitop commented 11 years ago

will push the changes in a minute, as it also requires an small change in the worker spec

kitop commented 11 years ago

Brandon, here's a gist with the correct settings: https://gist.github.com/kitop/5248674

Note that this happens only when using transactions in tests.

Important lines are https://gist.github.com/kitop/5248674#file-spec_helper-rb-L36-L39 and https://gist.github.com/kitop/5248674#file-email_worker_spec-rb-L3-L4

The :worker => true option could be specified in any describe, context, or it block. Hope it helps!

brandonhilkert commented 11 years ago

Thanks! https://github.com/brandonhilkert/sucker_punch#troubleshooting


http://brandonhilkert.com

On Tue, Mar 26, 2013 at 4:04 PM, Esteban Pastorino <notifications@github.com

wrote:

Brandon, here's a gist with the correct settings: https://gist.github.com/kitop/5248674

Note that this happens only when using transactions in tests.

Important lines are https://gist.github.com/kitop/5248674#file-spec_helper-rb-L36-L39 and https://gist.github.com/kitop/5248674#file-email_worker_spec-rb-L3-L4

The :worker => true option could be specified in any describe, context, or it block. Hope it helps!

— Reply to this email directly or view it on GitHubhttps://github.com/brandonhilkert/sucker_punch/issues/15#issuecomment-15484970 .

espen commented 11 years ago

I am not using database_cleaner but had this problem when running my tests. After reading this blog post I ended up specifying all calls to use the same AR connection.

Using test/unit instead of rspec.

Would be better if it was possible to run all async worker calls truly within the current code though. Not use threads at all.

class ActiveRecord::Base
  mattr_accessor :shared_connection
  @@shared_connection = nil
  def self.connection
    @@shared_connection || retrieve_connection
  end
end

# Forces all threads to share the same connection. This works on
# Capybara because it starts the web server in a thread.
ActiveRecord::Base.shared_connection = ActiveRecord::Base.connection