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

Now one can customize pid path for each work group. see #98 #99

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

Conversation

Hippoom
Copy link

@Hippoom Hippoom commented Mar 9, 2015

See detailed issue here

@Hippoom
Copy link
Author

Hippoom commented Mar 9, 2015

Seems to fail due to lack of guard, should I add development dependency in gemspec? I did this in my local env to pass all tests. But don't know is this the right way.

bundle exec rake
/home/travis/.rvm/gems/ruby-2.2.0/gems/guard-minitest-2.4.4/lib/minitest/guard_minitest_plugin.rb:4:in `require': cannot load such file -- guard (LoadError)
    from /home/travis/.rvm/gems/ruby-2.2.0/gems/guard-minitest-2.4.4/lib/minitest/guard_minitest_plugin.rb:4:in `'
    from /home/travis/.rvm/gems/ruby-2.2.0/gems/minitest-5.5.1/lib/minitest.rb:91:in `require'
    from /home/travis/.rvm/gems/ruby-2.2.0/gems/minitest-5.5.1/lib/minitest.rb:91:in `block in load_plugins'
    from /home/travis/.rvm/gems/ruby-2.2.0/gems/minitest-5.5.1/lib/minitest.rb:85:in `each'
    from /home/travis/.rvm/gems/ruby-2.2.0/gems/minitest-5.5.1/lib/minitest.rb:85:in `load_plugins'
    from /home/travis/.rvm/gems/ruby-2.2.0/gems/minitest-5.5.1/lib/minitest.rb:114:in `run'
    from /home/travis/.rvm/gems/ruby-2.2.0/gems/minitest-5.5.1/lib/minitest.rb:56:in `block in autorun'
Coverage report generated for RSpec to /home/travis/build/jondot/sneakers/coverage. 181 / 456 LOC (39.69%) covered.
rake aborted!
Command failed with status (1): [ruby -I"lib:spec"  "/home/travis/.rvm/rubies/ruby-2.2.0/lib/ruby/2.2.0/rake/rake_test_loader.rb" "spec/sneakers/cli_spec.rb" "spec/sneakers/concerns/logging_spec.rb" "spec/sneakers/concerns/metrics_spec.rb" "spec/sneakers/configuration_spec.rb" "spec/sneakers/publisher_spec.rb" "spec/sneakers/queue_spec.rb" "spec/sneakers/sneakers_spec.rb" "spec/sneakers/support/utils_spec.rb" "spec/sneakers/worker_handlers_spec.rb" "spec/sneakers/worker_spec.rb" ]
/home/travis/.rvm/gems/ruby-2.2.0/bin/ruby_executable_hooks:15:in `eval'
/home/travis/.rvm/gems/ruby-2.2.0/bin/ruby_executable_hooks:15:in `'
Tasks: TOP => default => test
(See full trace by running task with --trace)
The command "bundle exec rake" exited with 1.

@jondot
Copy link
Owner

jondot commented Mar 9, 2015

Thanks this commit looks great!
Yes - please feel free to add guard to the development dependencies, thanks!

@exec_hash = {
"WORKERS"=> worker_config[group_name]['classes'],
"WORKER_COUNT" => worker_config[group_name]["workers"].to_s,
"PID_PATH" => worker_config[group_name]['pid_path'].to_s
Copy link
Contributor

Choose a reason for hiding this comment

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

Would a test of this changed behavior be possible? Thanks in advance :)

Copy link
Author

Choose a reason for hiding this comment

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

It is not easy to write a test based on the current code since all config parsing is embedded in the fork block. I'm considering to introduce a WorkerGroupConfig to handle the parsing, something like this:

worker_config = new WorkerGroupConfiguration.New(YAML.load(File.read(worker_group_config_file)))
worker_config.keys.each do |group_name|
    @pids << fork do
        @exec_hash = config.env_vars(group_name)
        Kernel.exec(@exec_hash, @exec_string)
    end

Then I can test the config parsing with WorkerGroupConfiguration.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say you're right, a series of extract method refactorings would go a long way towards making Spawner.spawn testable.

As it stands, you'd have to stub out Kernel.exec, Signal.trap, and Process.waitall to be able to safely run this in a test - that's quite an interaction surface.

Copy link
Owner

Choose a reason for hiding this comment

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

Hey guys - should we say that forks/signal based code should be tested with an integration test as a general principle? or maybe there's a mock library for Ruby that would help mock these kind of interactions with the OS?

@michaelklishin
Copy link
Collaborator

@jondot @rud is this still relevant? @Hippoom can you please rebase this against master?

@rud
Copy link
Contributor

rud commented Sep 13, 2017

I'm not currently using sneakers, so I'm a bit out of the loop here. Thank you for reaching out, though.

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