Copilot commented on code in PR #8629:
URL: https://github.com/apache/hbase/pull/8629#discussion_r3957871561
##########
hbase-shell/src/main/ruby/irb/hirb.rb:
##########
@@ -51,6 +51,8 @@ def initialize(workspace = nil, interactive = true,
input_method = nil)
`stty icrnl <&2`
end
@interactive = interactive
+ # Errors must abort a script run to set the exit code, but not a live
prompt
+ @exit_on_error = !interactive || input_method.is_a?(FileInputMethod)
Review Comment:
Consider qualifying `FileInputMethod` (e.g., `IRB::FileInputMethod`) to make
constant resolution unambiguous. This avoids NameError surprises if `HIRB` is
defined as `class IRB::HIRB` (not lexically nested in `module IRB`) or if
constants are loaded differently in certain test/runtime environments.
##########
hbase-shell/src/main/ruby/irb/hirb.rb:
##########
@@ -161,17 +163,13 @@ def eval_input
rescue Interrupt => exc
rescue SystemExit, SignalException
raise
- rescue SyntaxError => exc
- # HBASE-27726: Ignore SyntaxError to prevent exiting Shell on
unexpected syntax.
- raise exc unless @interactive
- rescue NameError => exc
- raise exc unless @interactive
- # HBASE-26880: Ignore NameError to prevent exiting Shell on
mistyped commands.
rescue Exception => exc
# HBASE-26741: Raise exception so Shell::exception_handler can
catch it.
# This modifies this copied method from JRuby so that the HBase
shell can
# manage the exception and set a proper exit code on the process.
- raise exc
+ # Otherwise keep the session alive and report the error, as the
shell did before
+ # HBASE-26741. Supersedes HBASE-26880 (NameError) and HBASE-27726
(SyntaxError).
+ raise exc if @exit_on_error
Review Comment:
Inside a `rescue`, `raise exc` can alter the exception’s backtrace compared
to a bare `raise`. If the intent is to re-raise the same exception with its
original backtrace for accurate debugging and error reporting, prefer `raise`
(without an argument) when `@exit_on_error` is true.
##########
hbase-shell/src/test/ruby/shell/general_test_cluster.rb:
##########
@@ -226,4 +227,24 @@ def readable_after_eof?
assert_match(/WARN: 'scan' is a reserved HBase command/, err_output)
assert_match(/WARN: 'processlist' is a reserved HBase command/, err_output)
end
+
+ def new_hirb(input_method)
+ IRB.setup(__FILE__) unless IRB.conf[:IRB_NAME]
+ IRB::HIRB.new(@shell.workspace, true, input_method)
+ end
+
+ define_test 'Shell::Shell should keep an interactive session alive on any
error' do
+ hirb = new_hirb(MockInputMethod.new(["1 + '2'\n", "my_var = 5\n"]))
+ capture_stdout { hirb.eval_input }
+ assert_equal(5, hirb.context.workspace.binding.local_variable_get(:my_var))
+ end
+
+ define_test 'Shell::Shell should abort a script on error even when
interactive' do
+ file = Tempfile.new(['hirb_test', '.rb'])
+ file.write("1 + '2'\n")
+ file.close
+ # interactive is true, as it is for `hbase shell script.rb` without -n
+ hirb = new_hirb(IRB::HBaseLoader.file_for_load(file.path))
+ assert_raise(TypeError) { capture_stdout { hirb.eval_input } }
Review Comment:
Tempfile is created without ensuring it’s unlinked. This can accumulate temp
files in CI or repeated local runs. Prefer using the block form (so it
auto-closes/unlinks) or explicitly `unlink` it in an `ensure` after the test
completes.
##########
hbase-shell/src/test/ruby/shell/general_test_cluster.rb:
##########
@@ -226,4 +227,24 @@ def readable_after_eof?
assert_match(/WARN: 'scan' is a reserved HBase command/, err_output)
assert_match(/WARN: 'processlist' is a reserved HBase command/, err_output)
end
+
+ def new_hirb(input_method)
+ IRB.setup(__FILE__) unless IRB.conf[:IRB_NAME]
+ IRB::HIRB.new(@shell.workspace, true, input_method)
+ end
Review Comment:
`IRB.setup` mutates global IRB configuration, and doing that inside a helper
used by multiple tests can create ordering/cross-test coupling. Consider moving
IRB initialization to a suite/setup hook (and, if needed, restoring any
modified IRB.conf values in teardown) so individual tests don’t implicitly
depend on shared global state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
