-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-24722 Address hbase-shell commands with unintentional return values #2058
Conversation
- Prior to this commit, there were 13 commands that unintentionally return the number of lines they print (usually one). This commit ensures that they return the value documented by the help text, or nil if there is not a simple logical value to return. - Fixes 6 hbase-shell commands that return String rather than TrueClass or FalseClass - Use double-bang to cast truthy values to TrueClass and FalseClass so that ruby's to_s can reliably print true or false without using ternary operators - Updates tests for is_disabled, is_enabled, disable_rpc_throttle, enable_rpc_throttle, disable_exceed_throttle_quota, enable_exceed_throttle_quota, clear_deadservers, snapshot_cleanup_switch, snapshot_cleanup_enabled, and balancer to check return values - Adds new tests for balance_switch, balancer_enabled, normalizer_switch, normalizer_enabled, catalog_janitor_switch, catalogjanitor_enabled, cleaner_chore_switch, cleaner_chore_enabled, splitormerge_switch, and splitormerge_enabled
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -31,7 +31,7 @@ def help | |||
end | |||
|
|||
def command(enableDisable) | |||
prev_state = admin.balance_switch(enableDisable) ? 'true' : 'false' | |||
prev_state = !!admin.balance_switch(enableDisable) |
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.
Nice idiom convertion (java to ruby)
|
||
output = capture_stdout { result = command(:splitormerge_enabled, 'MERGE') } | ||
assert(output.include?('true')) | ||
assert(result == true) |
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.
Sweet
…2058) - Prior to this commit, there were 13 commands that unintentionally return the number of lines they print (usually one). This commit ensures that they return the value documented by the help text, or nil if there is not a simple logical value to return. - Fixes 6 hbase-shell commands that return String rather than TrueClass or FalseClass - Use double-bang to cast truthy values to TrueClass and FalseClass so that ruby's to_s can reliably print true or false without using ternary operators - Updates tests for is_disabled, is_enabled, disable_rpc_throttle, enable_rpc_throttle, disable_exceed_throttle_quota, enable_exceed_throttle_quota, clear_deadservers, snapshot_cleanup_switch, snapshot_cleanup_enabled, and balancer to check return values - Adds new tests for balance_switch, balancer_enabled, normalizer_switch, normalizer_enabled, catalog_janitor_switch, catalogjanitor_enabled, cleaner_chore_switch, cleaner_chore_enabled, splitormerge_switch, and splitormerge_enabled signed-off-by: stack <stack@apache.org>
…2058) - Prior to this commit, there were 13 commands that unintentionally return the number of lines they print (usually one). This commit ensures that they return the value documented by the help text, or nil if there is not a simple logical value to return. - Fixes 6 hbase-shell commands that return String rather than TrueClass or FalseClass - Use double-bang to cast truthy values to TrueClass and FalseClass so that ruby's to_s can reliably print true or false without using ternary operators - Updates tests for is_disabled, is_enabled, disable_rpc_throttle, enable_rpc_throttle, disable_exceed_throttle_quota, enable_exceed_throttle_quota, clear_deadservers, snapshot_cleanup_switch, snapshot_cleanup_enabled, and balancer to check return values - Adds new tests for balance_switch, balancer_enabled, normalizer_switch, normalizer_enabled, catalog_janitor_switch, catalogjanitor_enabled, cleaner_chore_switch, cleaner_chore_enabled, splitormerge_switch, and splitormerge_enabled signed-off-by: stack <stack@apache.org>
Resolves https://issues.apache.org/jira/browse/HBASE-24722
Changelog
number of lines they print (usually one). This commit ensures that they
return the value documented by the help text, or nil if there is not a simple
logical value to return.
FalseClass
ruby's to_s can reliably print true or false without using ternary operators
enable_rpc_throttle, disable_exceed_throttle_quota,
enable_exceed_throttle_quota, clear_deadservers, snapshot_cleanup_switch,
snapshot_cleanup_enabled, and balancer to check return values
normalizer_enabled, catalog_janitor_switch, catalogjanitor_enabled,
cleaner_chore_switch, cleaner_chore_enabled, splitormerge_switch, and
splitormerge_enabled
Note that the many assertions with explicit comparisons to booleans are intentional (ie.
assert(return_value == false)
). In ruby, this test makes sure that return value is actually an instance of TrueClass or FalseClass, which makes these tests much more potent. Since every ruby object can be cast to a boolean, we don't want to justassert(return_value)
.Testing