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

Fix activejob integration test #19470

Closed
wants to merge 3 commits into from

Conversation

troter
Copy link
Contributor

@troter troter commented Mar 23, 2015

  • fix activejob integration test's queue_adapter initialization.
  • avoid double initialization error caused to sidekiq

In 4-2-stable, initialzers related ActiveJob are executed in this order.

    1. active_job.set_config
    2. load_config_initializers

In master, same initializers are executed in this order.

    1. load_config_initializers
    2. active_job.set_config

As a result, ActiveJob's integration test always use InlineAdapter in
master.
Sidekiq::CLI#boot_system require "#{dummy_app_path}/config/environment.rb".
But this file has already been required in 'test/support/integration/helper.rb'.
@rafaelfranca
Copy link
Member

If what you wrote about initializers orders is true this is a bug in the code, not in the test suite. Also can we not call Sidekiq::CLI#boot_system?

@troter
Copy link
Contributor Author

troter commented Mar 23, 2015

@rafaelfranca thank you for your comments.

If what you wrote about initializers orders is true this is a bug in the code, not in the test suite.

This is initializer dipendencies of 4-2-stable and master.

active_support.halt_callback_chains_on_return_false is seem to cause.

Also can we not call Sidekiq::CLI#boot_system?

Sidekiq::CLI#run call Sidekiq::CLI#boot_system at first.

I guess that it is possible to call a Sidekiq::Launcher directly.

@rafaelfranca
Copy link
Member

Could you print the actual initializer order instead of the order by name and run the compare them?

@mperham what do you think about troter@3184b96?

@rafaelfranca
Copy link
Member

Yeah, just found that halt_callback_chains_on_return_false is the cause of that problem. I'll fix it.

@mperham
Copy link
Contributor

mperham commented Apr 7, 2015

@rafaelfranca I would use Sidekiq::Launcher. The CLI code is not designed to run within another process.

@rafaelfranca
Copy link
Member

@troter the first commit should not be necessary after 0a120a8. Now could you use Sidekiq::Launcher?

rafaelfranca added a commit that referenced this pull request Apr 8, 2015
Sidekiq::CLI#boot_system require "#{dummy_app_path}/config/environment.rb".
But this file has already been required in'test/support/integration/helper.rb'.
This patch will change to use Sidekiq::Launcher directly.
@troter
Copy link
Contributor Author

troter commented Apr 10, 2015

@rafaelfranca I add commit 9c047c8, this change use Sidekiq::Launcher directly. please re-review it.

@rafaelfranca
Copy link
Member

Thank you. Merged at b126e7a

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

Successfully merging this pull request may close these issues.

None yet

3 participants