Prevent infinite loop in Command::Base#step & add configurable wait time
#217
Conversation
WalkthroughThis update introduces enhancements to the codebase, including a new RSpec linting configuration, improvements to the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CommandBase
Caller->>CommandBase: Call #step(message, options)
alt retries enabled
loop until max_retry_count
CommandBase->>CommandBase: Execute block
alt success
CommandBase-->>Caller: Return result
else failure
CommandBase->>CommandBase: Increment run_count
end
end
else retries disabled
CommandBase->>CommandBase: Execute block once
CommandBase-->>Caller: Return result
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
zzaakiirr
force-pushed
the
216-prevent-infinite-loop-for-command-step-method
branch
from
65b24a2 to
9c921c1
Compare
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
lib/command/base.rb (1)
486-486: Enhance documentation for new parameters.The new parameters
max_retry_countandwaitshould be documented in the method's docstring to clarify their purpose and usage.# Adds a step with a message, retries on failure if specified. # @param message [String] the message to display # @param abort_on_error [Boolean] whether to abort on error # @param retry_on_failure [Boolean] whether to retry on failure # @param max_retry_count [Integer] the maximum number of retries # @param wait [Integer] the duration to wait between retries (in seconds) def step(message, abort_on_error: true, retry_on_failure: false, max_retry_count: 5, wait: 1)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .rubocop.yml (1 hunks)
- lib/command/base.rb (1 hunks)
- spec/command/base_spec.rb (1 hunks)
- spec/support/command_helpers.rb (1 hunks)
Additional comments not posted (4)
.rubocop.yml (1)
23-26: LGTM! The new configuration forRSpec/NestedGroupsis correct.The configuration enables the rule and sets the maximum allowed nested groups to 5, which helps improve test organization and maintainability.
spec/command/base_spec.rb (2)
1-12: LGTM! The initial setup and subject definition are appropriate.The setup and subject definition correctly prepare the test environment for testing the
Command::Baseclass.
14-60: LGTM! The tests for thestepmethod are well-structured and cover important edge cases.The tests ensure that the retry logic works correctly with different configurations. Consider adding tests for configurable wait times to further enhance coverage.
spec/support/command_helpers.rb (1)
206-214: LGTM! Thesuppress_outputmethod is well-implemented.The method correctly captures the original streams, replaces them with temporary files, and ensures that the original streams are restored after the block has executed, even if an error occurs.
zzaakiirr
force-pushed
the
216-prevent-infinite-loop-for-command-step-method
branch
from
9c921c1 to
15baea1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .rubocop.yml (1 hunks)
- lib/command/base.rb (1 hunks)
- spec/command/base_spec.rb (1 hunks)
- spec/support/command_helpers.rb (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- .rubocop.yml
- lib/command/base.rb
- spec/command/base_spec.rb
- spec/support/command_helpers.rb
There was a problem hiding this comment.
Hm, I'm not sure that we want to do this. There are some cases where step may fail, but we do want it to keep retrying infinitely, because it will eventually succeed. I'll see if I can locate those cases and what can be done about them, otherwise this would be a breaking change.
@rafaelgomesxyz We can set |
There was a problem hiding this comment.
@zzaakiirr Setting Float::INFINITY as the default value makes it so that this PR doesn't really change anything.
Let's leave this on hold for a bit, I'll get back to it in a few days.
@rafaelgomesxyz Yes, it doesn't change current behaviour, but method becomes more flexible - we already have issue where we can profit from configurable wait time & retry count - we shouldn't wait infinite time if domain update fails, it's better to fail with error, IMHO |
zzaakiirr
force-pushed
the
216-prevent-infinite-loop-for-command-step-method
branch
from
15baea1 to
ccf58b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .rubocop.yml (1 hunks)
- lib/command/base.rb (1 hunks)
- spec/command/base_spec.rb (1 hunks)
- spec/support/command_helpers.rb (1 hunks)
Additional comments not posted (8)
.rubocop.yml (1)
24-26: LGTM! EnablingRSpec/NestedGroupswith a limit of 5.This change promotes better readability and maintainability of test code by enforcing a structure on nested example groups.
spec/command/base_spec.rb (5)
7-7: Consider usingletforcommanddeclaration.The
commandis not being tested directly, so consider usingletinstead ofsubject.
27-27: Nitpick: Use parentheses forexpectarguments.Consider using parentheses for better readability:
expect(run_count).to eq(3)
31-31: Clarify variable naming for options.Consider using a more descriptive variable name like
options_with_max_retry_countto avoid confusion.
41-41: Nitpick: Use parentheses forexpectarguments.Consider using parentheses for better readability:
expect(run_count).to eq(1 + 1) # 1 run and 1 retry after fail
57-57: Nitpick: Use parentheses forexpectarguments.Consider using parentheses for better readability:
expect(run_count).to eq(1)spec/support/command_helpers.rb (1)
210-218: LGTM! Suppressing output withsuppress_output.The
suppress_outputmethod is well-implemented, effectively managing console output during tests.lib/command/base.rb (1)
477-477: Verify default values formax_retry_countandwait.The default values for
max_retry_count(1000) andwait(1) should be reviewed to ensure they align with expected use cases. Consider the feedback from reviewers about setting a higher default formax_retry_countto accommodate longer wait times.Run the following script to verify the usage of
stepmethod and ensure that the default values align with the expected use cases:Verification successful
Default Values for
max_retry_countandwaitAre AppropriateThe
stepmethod is widely used across the codebase, and its behavior, including default values, is tested inspec/command/base_spec.rb. This suggests that the defaults are suitable for the expected use cases.
- Files with
stepmethod usage:
spec/command/base_spec.rblib/command/delete.rblib/command/maintenance_set_page.rblib/command/run.rb- And others...
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `step` method and ensure the default values align with expected use cases. # Test: Search for the `step` method usage. Expect: Occurrences should align with default values. rg --type ruby -A 5 $'step('Length of output: 90
Script:
#!/bin/bash # Description: Verify the usage of `step` method and ensure the default values align with expected use cases. # Test: Search for the `step` method usage. Expect: Occurrences should align with default values. rg --type ruby -A 5 'step\('Length of output: 14861
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- spec/command/base_spec.rb (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- spec/command/base_spec.rb
zzaakiirr
force-pushed
the
216-prevent-infinite-loop-for-command-step-method
branch
from
046349f to
7c768e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- lib/command/base.rb (2 hunks)
- spec/command/base_spec.rb (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- lib/command/base.rb
- spec/command/base_spec.rb
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- spec/command/base_spec.rb (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- spec/command/base_spec.rb
zzaakiirr
deleted the
216-prevent-infinite-loop-for-command-step-method
branch
What does this PR do?
This PR improves
Command::Base#stepmethodSummary by CodeRabbit
New Features
Bug Fixes
Documentation