Remove arguments from perform method #140
Conversation
|
The linter fails because code uses Ruby 2.5 syntax and Rubocop is set to lint against 2.4 (the same as in gemspec https://github.com/bensheldon/good_job/blob/main/good_job.gemspec#L48) but there are no tests for Ruby 2.4. |
|
@morgoth Oof, sorry you got bitten by that inconsistency. Let's drop support for Ruby 2.4; it looks like Ruby 2.4 EOLed on March 31, 2020. Could you please remove the references to 2.4, and change the Linter config to 2.5 as its own PR? |
There was a problem hiding this comment.
This is fantastic! 🙌
I left some feedback on the tests, specifically to use RSpec message stubs instead of manually setting and resetting global values. I have to apologize (and express my gratitude to you for working in them) because those tests could be more exemplary.
Once you fix the linter issue, and make the changes to the tests, this is ready to go 👍
spec/lib/good_job/job_spec.rb
Outdated
| @@ -204,11 +204,16 @@ def perform(result_value = nil, raise_error: false) | |||
| end | |||
|
|
|||
| it 'can preserve the job' do | |||
| good_job.perform(destroy_after: false) | |||
| current_preserve_job_records = GoodJob.preserve_job_records | |||
| GoodJob.preserve_job_records = true | |||
There was a problem hiding this comment.
The RSpec-y way to do this is with a message stub which will be automatically cleaned up and reset at the end of the test example: allow(GoodJob).to receive(:preserve_job_records).and_return(true)
| if GoodJob.preserve_job_records | ||
| save! | ||
| else | ||
| destroy! |
There was a problem hiding this comment.
This reads so much better now with the positive condition coming first 🎉
spec/lib/good_job/job_spec.rb
Outdated
| current_reperform_jobs_on_standard_error = GoodJob.reperform_jobs_on_standard_error | ||
| current_preserve_job_records = GoodJob.preserve_job_records | ||
| GoodJob.reperform_jobs_on_standard_error = true | ||
| GoodJob.preserve_job_records = true |
There was a problem hiding this comment.
Using message stubs allow this to be moved up to the context block so that they are applied to all tests within that context block and avoid test setup repetition.
context 'when true' do
before do
allow(GoodJob).to receive(:reperform_jobs_on_standard_error).and_return(true)
allow(GoodJob).to receive(:preserve_job_records).and_return(true)
end
#....
morgoth
force-pushed
the
remove-args-from-perform
branch
from
5ca59ca
to
72ef163
Compare
|
@bensheldon updated and tests are green now. Thanks |
|
@morgoth Thank you! 🙌 |
Follow up to #136 (comment)