-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add CLI Run Test - Write Pid, System Boot, Print Banner #4039
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
Conversation
5b39f09
to
a15fb94
Compare
Don't got overboard on test coverage. The code doesn't change often and it's far easier to test manually once than to construct and maintain some really complicated test harness. |
I just try to understand the code and learn that way, but generally I agree ^_^ |
@mperham Are you comfortable with "appraisals" presence? |
For now, sure. I will remove it if it becomes a problem. |
e803640
to
840a919
Compare
@mperham Please let me know if you are fine with the current changes and commits history 🙇 |
Sidekiq.logger.level = Logger::ERROR | ||
|
||
REDIS_URL = ENV['REDIS_URL'] || 'redis://localhost/15' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want the test suite to use the default Redis localhost:6379/0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I may return back
ENV['REDIS_URL'] ||= 'redis://localhost/15'
or without customisation oportunity
ENV['REDIS_URL'] = 'redis://localhost/15'
That should be enough I guess. BTW, why 15
? Is it kind of virtual host in RabbitMQ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't want to be a dick, but we already have tests which rely on default Redis host ^_^
https://github.com/mperham/sidekiq/blob/master/test/test_redis_connection.rb#L84
https://github.com/mperham/sidekiq/blob/master/test/test_redis_connection.rb#L126
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yeah, I want people's default redis database to be untouched by Sidekiq's test suite. We'll need to fix those tests (or make them read only)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already done this, Mike ^_^
1940a5d
to
ea48409
Compare
@mperham Addressed all your concerns I think. Let me know if there's anything else you can think of. |
Thank you for this! I will add a changelog entry to give you credit. |
@mperham Thank you 🙇 It's worth to note about |
I tried to cover as much as I can in
#run
method with these changes. I'm still thinking how to test daemonized version...Refactgoring
Sidekiq::CLI#launch
method to detach actual launcher start from the system preparations (write pid, boot system, print banner) to test in isolationhelper
Improvements
spec/dummy
for theSidekiq::Rails
engine integration testappraisal
to testsidekiq
integration with differentrails
versions (ATM 4 and 5). Appraisals may be used to test either different versions of thesidekiq
dependencies in the future.minitest-focus
minitest-reporters
for convenient work with the tests in RubyMineRelated PRs