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

Configure ActiveJob's queue adapter to DelayedJob #526

Closed
wants to merge 2 commits into from
Closed

Configure ActiveJob's queue adapter to DelayedJob #526

wants to merge 2 commits into from

Conversation

ventsislaf
Copy link
Contributor

This removes the step to manually configure the ActiveJob's adapter.

Closes #520

config.active_job.queue_adapter = :delayed_job
RUBY

inject_into_class 'config/application.rb', 'Application', config

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

This removes the step to manually configure the `ActiveJob`'s adapter.

Closes #520
@mcmire
Copy link
Contributor

mcmire commented Feb 28, 2015

This seems good to me, anyone have any thoughts?

@tute
Copy link
Contributor

tute commented Feb 28, 2015

👍 from me.

@mcmire do we have any delayed job specifics in other parts of the project? See Derek's comment: #520 (comment)

@mcmire
Copy link
Contributor

mcmire commented Feb 28, 2015

That's a good question. I don't know as I haven't studied the Suspenders code in a long time.

@tute
Copy link
Contributor

tute commented Feb 28, 2015

Looping in @derekprior for that. Next week I'll test and merge!

@ventsislaf
Copy link
Contributor Author

Thank you for the fast feedback guys.

What about this piece of code? https://github.com/thoughtbot/suspenders/blob/master/templates/background_jobs_rspec.rb
Do you think we could make this not adapter specific? Also I was wondering whether to set the ActiveJob's adapter to :test in the test environment.
http://api.rubyonrails.org/classes/ActiveJob/QueueAdapters/TestAdapter.html

I think I need to look more into ActiveJob.

@ventsislaf
Copy link
Contributor Author

Another option would be to use the inline adapter when running features which will execute the jobs immediately.

Any thoughts are welcome!

@jferris
Copy link
Contributor

jferris commented Mar 2, 2015

Another option would be to use the inline adapter when running features which will execute the jobs immediately.

I think this would have the same effect as the current background_jobs_rspec.rb file. The purpose is just to make it so that background jobs will run during integration tests.

@ventsislaf
Copy link
Contributor Author

I think this would have the same effect as the current background_jobs_rspec.rb file. The purpose is just to make it so that background jobs will run during integration tests.

I agree. I'm just wondering here... is there any reason to make it only for integration tests and not for the whole test environment?

@jferris
Copy link
Contributor

jferris commented Mar 2, 2015

I agree. I'm just wondering here... is there any reason to make it only for integration tests and not for the whole test environment?

I think the theory is:

In integration tests, we assume that you generally want to test everything working together, so it makes sense to just run the background jobs and treat the fact that they're backgrounded as an implementation detail. When your test is "the credit card is charged," you don't care if that happens in an HTTP request or a background job.

In unit tests, we assume that you're testing a specific class, and any interactions with other classes are managed by the test. That means that you probably don't need the actual background job to run; you just need to verify that it did run. We usually use mocks to verify this.

In reality, tests tend to be at least somewhat of a blend of both, but this default approach to testing background jobs seems to be reasonably intuitive and has worked out well for us so far.

@ventsislaf
Copy link
Contributor Author

In unit tests, we assume that you're testing a specific class, and any interactions with other classes are managed by the test. That means that you probably don't need the actual background job to run; you just need to verify that it did run. We usually use mocks to verify this.

I know. That's why I asked whether is worthy to set this only for the integration tests. We would anyway mock the background jobs in the unit tests and I couldn't think of a reason not to set the inline adapter in the config/test/environment.rb.

Don't take me wrong. I don't mind just to change the existing test helper if that's what we want.

@jferris
Copy link
Contributor

jferris commented Mar 3, 2015

I know. That's why I asked whether is worthy to set this only for the integration tests.

Ah, I see. I misunderstood your original question.

I'd be open to trying :inline globally in tests to see how that works out. It's simpler and I can't think of situations where it would be likely to cause problems.

* Use `:inline` queue adapter in test environment to execute the
  background jobs immediately
* Remove the background jobs template because it's not necessary anymore
@ventsislaf
Copy link
Contributor Author

Any thoughts?

@jferris
Copy link
Contributor

jferris commented Mar 6, 2015

This looks good to me.

@tute any thoughts?

configure_application_file(
"config.active_job.queue_adapter = :delayed_job"
)
configure_environment "test", "config.active_job.queue_adapter = :inline"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is cool!

@jessieay
Copy link
Contributor

jessieay commented Mar 6, 2015

This looks good to me! I didn't know about ActiveJob so thanks for the educational PR!

@ventsislaf
Copy link
Contributor Author

@jessieay I'm glad that you find my PR is useful.

@tute
Copy link
Contributor

tute commented Mar 7, 2015

Merged in fce1401. Thank you! :)

@tute tute closed this Mar 7, 2015
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.

ActiveJob
6 participants