From c258ee0f2832a64e2e83814416289a6e0f7f4d06 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Thu, 9 Mar 2023 18:07:30 +0800 Subject: [PATCH 1/8] Avoid raising debuggee not finished error if assertions failed When assertions failed, we should let the AssertionFailedError surface and not calling flunk so we can get correct failure messages. --- test/support/protocol_test_case.rb | 10 +++++++-- test/support/protocol_test_case_test.rb | 27 +++++++++++++++++++------ 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/test/support/protocol_test_case.rb b/test/support/protocol_test_case.rb index 1deb5cd98..acdcae65c 100644 --- a/test/support/protocol_test_case.rb +++ b/test/support/protocol_test_case.rb @@ -334,9 +334,12 @@ def execute_dap_scenario scenario attach_to_dap_server scenario.call + rescue Test::Unit::AssertionFailedError => e + is_assertion_failure = true + raise e ensure kill_remote_debuggee test_info - if test_info.failed_process + if test_info.failed_process && !is_assertion_failure flunk create_protocol_message "Expected the debuggee program to finish" end # Because the debuggee may be terminated by executing the following operations, we need to run them after `kill_remote_debuggee` method. @@ -365,9 +368,12 @@ def execute_cdp_scenario_ scenario res = find_response :method, 'Debugger.paused', 'C e + is_assertion_failure = true + raise e ensure kill_remote_debuggee test_info - if test_info.failed_process + if test_info.failed_process && !is_assertion_failure flunk create_protocol_message "Expected the debuggee program to finish" end # Because the debuggee may be terminated by executing the following operations, we need to run them after `kill_remote_debuggee` method. diff --git a/test/support/protocol_test_case_test.rb b/test/support/protocol_test_case_test.rb index d9e687f9b..4c132b8ac 100644 --- a/test/support/protocol_test_case_test.rb +++ b/test/support/protocol_test_case_test.rb @@ -3,14 +3,29 @@ require_relative 'protocol_test_case' module DEBUGGER__ - class TestFrameworkTestHOge < ProtocolTestCase - PROGRAM = <<~RUBY - 1| a=1 - RUBY - + class TestProtocolTestCase < ProtocolTestCase def test_the_test_fails_when_debuggee_doesnt_exit + program = <<~RUBY + 1| a=1 + RUBY + assert_fail_assertion do - run_protocol_scenario PROGRAM do + run_protocol_scenario program do + end + end + end + + def test_the_assertion_failure_takes_presedence_over_debuggee_not_exiting + program = <<~RUBY + 1| a = 2 + 2| b = 3 + RUBY + + assert_raise_message(/<\"100\"> expected but was/) do + run_protocol_scenario program do + req_add_breakpoint 2 + req_continue + assert_repl_result({value: '100', type: 'Integer'}, 'a') end end end From 64552e2013338f6af03031dceed44494a1591eb1 Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Fri, 10 Mar 2023 13:09:51 -0500 Subject: [PATCH 2/8] Fix test builder --- CONTRIBUTING.md | 4 ++-- test/tool/test_builder.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 22c8bfcf0..68ca46798 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -149,10 +149,10 @@ If the file already exists, **only method** will be added to it. ```ruby # frozen_string_literal: true -require_relative '../support/test_case' +require_relative '../support/console_test_case' module DEBUGGER__ - class FooTest < TestCase + class FooTest < ConsoleTestCase def program <<~RUBY 1| module Foo diff --git a/test/tool/test_builder.rb b/test/tool/test_builder.rb index be48a5514..45fe10843 100644 --- a/test/tool/test_builder.rb +++ b/test/tool/test_builder.rb @@ -157,7 +157,7 @@ def #{@method} def create_scenario_and_program <<-TEST.chomp - class #{@class}#{@current_time} < TestCase + class #{@class}#{@current_time} < ConsoleTestCase def program <<~RUBY #{format_program} From ec4e2f7980ad6c49068a3742fade38ee9e0aa579 Mon Sep 17 00:00:00 2001 From: Naoto Ono Date: Sun, 5 Mar 2023 02:10:38 +0900 Subject: [PATCH 3/8] DAP: allow custom request extension in ThreadClient class --- lib/debug/server_dap.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/debug/server_dap.rb b/lib/debug/server_dap.rb index 0a2afa71c..63b75827a 100644 --- a/lib/debug/server_dap.rb +++ b/lib/debug/server_dap.rb @@ -973,7 +973,11 @@ def process_dap args } else - raise "Unknown req: #{args.inspect}" + if respond_to? mid = "request_#{type}" + __send__ mid, req + else + raise "Unknown request: #{args.inspect}" + end end end From e29fabaecde4086020953de56e833bf3b076f7b5 Mon Sep 17 00:00:00 2001 From: Naoto Ono Date: Tue, 21 Mar 2023 00:31:48 +0900 Subject: [PATCH 4/8] DAP: introduce Rdbg Record Inspector --- lib/debug/server_dap.rb | 65 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/lib/debug/server_dap.rb b/lib/debug/server_dap.rb index 63b75827a..0cc76f337 100644 --- a/lib/debug/server_dap.rb +++ b/lib/debug/server_dap.rb @@ -468,6 +468,10 @@ def process send_event :terminated unless @sock.closed? end + def request_rdbgRecordInspector req + @q_msg << req + end + ## called by the SESSION thread def respond req, res @@ -664,6 +668,43 @@ def process_protocol_request req end end + def request_rdbgRecordInspector req + cmd = req.dig('arguments', 'command') + case cmd + when 'enable' + request_tc [:record, :on] + @ui.respond req, {} + when 'disable' + request_tc [:record, :off] + @ui.respond req, {} + when 'step' + tid = req.dig('arguments', 'threadId') + count = req.dig('arguments', 'count') + if tc = find_waiting_tc(tid) + tc << [:step, :in, count] + else + fail_response req + end + when 'stepBack' + tid = req.dig('arguments', 'threadId') + count = req.dig('arguments', 'count') + if tc = find_waiting_tc(tid) + tc << [:step, :back, count] + else + fail_response req + end + when 'collect' + tid = req.dig('arguments', 'threadId') + if tc = find_waiting_tc(tid) + tc << [:dap, :rdbgRecordInspector, req] + else + fail_response req + end + else + raise "Unknown command #{cmd}" + end + end + def dap_event args # puts({dap_event: args}.inspect) type, req, result = args @@ -711,6 +752,10 @@ def dap_event args end when :completions @ui.respond req, result + + # custom request + when :rdbgRecordInspector + @ui.respond req, result else raise "unsupported: #{args.inspect}" end @@ -981,6 +1026,26 @@ def process_dap args end end + def request_rdbgRecordInspector req + logs = [] + log_index = nil + unless @recorder.nil? + log_index = @recorder.log_index + @recorder.log.each{|frames| + crt_frame = frames[0] + logs << { + name: crt_frame.name, + location: { + path: crt_frame.location.path, + line: crt_frame.location.lineno, + }, + depth: crt_frame.frame_depth + } + } + end + event! :dap_result, :rdbgRecordInspector, req, logs: logs, stoppedIndex: log_index + end + def search_const b, expr cs = expr.delete_prefix('::').split('::') [Object, *b.eval('::Module.nesting')].reverse_each{|mod| From 49e37f95a50ce46b5dc9198e69604f93ff6d869a Mon Sep 17 00:00:00 2001 From: Koichi Sasada Date: Wed, 22 Mar 2023 11:27:43 +0900 Subject: [PATCH 5/8] Revert "DAP: introduce Rdbg Record Inspector" This reverts commit e29fabaecde4086020953de56e833bf3b076f7b5. --- lib/debug/server_dap.rb | 65 ----------------------------------------- 1 file changed, 65 deletions(-) diff --git a/lib/debug/server_dap.rb b/lib/debug/server_dap.rb index 0cc76f337..63b75827a 100644 --- a/lib/debug/server_dap.rb +++ b/lib/debug/server_dap.rb @@ -468,10 +468,6 @@ def process send_event :terminated unless @sock.closed? end - def request_rdbgRecordInspector req - @q_msg << req - end - ## called by the SESSION thread def respond req, res @@ -668,43 +664,6 @@ def process_protocol_request req end end - def request_rdbgRecordInspector req - cmd = req.dig('arguments', 'command') - case cmd - when 'enable' - request_tc [:record, :on] - @ui.respond req, {} - when 'disable' - request_tc [:record, :off] - @ui.respond req, {} - when 'step' - tid = req.dig('arguments', 'threadId') - count = req.dig('arguments', 'count') - if tc = find_waiting_tc(tid) - tc << [:step, :in, count] - else - fail_response req - end - when 'stepBack' - tid = req.dig('arguments', 'threadId') - count = req.dig('arguments', 'count') - if tc = find_waiting_tc(tid) - tc << [:step, :back, count] - else - fail_response req - end - when 'collect' - tid = req.dig('arguments', 'threadId') - if tc = find_waiting_tc(tid) - tc << [:dap, :rdbgRecordInspector, req] - else - fail_response req - end - else - raise "Unknown command #{cmd}" - end - end - def dap_event args # puts({dap_event: args}.inspect) type, req, result = args @@ -752,10 +711,6 @@ def dap_event args end when :completions @ui.respond req, result - - # custom request - when :rdbgRecordInspector - @ui.respond req, result else raise "unsupported: #{args.inspect}" end @@ -1026,26 +981,6 @@ def process_dap args end end - def request_rdbgRecordInspector req - logs = [] - log_index = nil - unless @recorder.nil? - log_index = @recorder.log_index - @recorder.log.each{|frames| - crt_frame = frames[0] - logs << { - name: crt_frame.name, - location: { - path: crt_frame.location.path, - line: crt_frame.location.lineno, - }, - depth: crt_frame.frame_depth - } - } - end - event! :dap_result, :rdbgRecordInspector, req, logs: logs, stoppedIndex: log_index - end - def search_const b, expr cs = expr.delete_prefix('::').split('::') [Object, *b.eval('::Module.nesting')].reverse_each{|mod| From 6d0c2672a5afa78b9b729b405a1053e41b4b8030 Mon Sep 17 00:00:00 2001 From: Koichi Sasada Date: Wed, 22 Mar 2023 18:05:08 +0900 Subject: [PATCH 6/8] relax authority check to pass on the Windows. fix https://github.com/ruby/vscode-rdbg/issues/169 --- lib/debug/config.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/debug/config.rb b/lib/debug/config.rb index 7ffdd741c..6cd23761d 100644 --- a/lib/debug/config.rb +++ b/lib/debug/config.rb @@ -425,8 +425,9 @@ def self.check_dir_authority path unless (dir_uid = fs.uid) == (uid = Process.uid) raise "#{path} uid is #{dir_uid}, but Process.uid is #{uid}" end - unless (dir_mode = fs.mode) == 040700 # 4: dir, 7:rwx - raise "#{path}'s mode is #{dir_mode.to_s(8)} (should be 040700)" + + if fs.world_writable? && !fs.sticky? + raise "#{path} is world writable but not sticky" end path From 118c5da90219e8efe4b91a789667147371c6fd81 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Thu, 9 Mar 2023 21:43:41 +0800 Subject: [PATCH 7/8] Use better approach to signal remote process' exit status 1. `remote_info.failed_process` stores symbol but we only use the value's presence to check if the process is force-killed. 2. The force-killed status is directly controlled by `kill_safely` through `kill_remote_debuggee`, which is directly called right before we check the status with `remote_info.failed_process`. Combining the two, we can just let `kill_safely` and `kill_remote_debuggee` to return the force-killed status and not storing it in `remote_info`, which already contains a bunch of information. This also eliminates the need to pass `test_info` to `kill_safely`, which makes related code easier to understand and maintain. --- test/support/console_test_case.rb | 2 +- test/support/protocol_test_case.rb | 6 ++---- test/support/test_case.rb | 15 ++++++++------- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/test/support/console_test_case.rb b/test/support/console_test_case.rb index 081b3f335..15230ccfa 100644 --- a/test/support/console_test_case.rb +++ b/test/support/console_test_case.rb @@ -200,7 +200,7 @@ def run_test_scenario cmd, test_info # kill debug console process read.close write.close - kill_safely pid, :debugger, test_info + kill_safely pid end end end diff --git a/test/support/protocol_test_case.rb b/test/support/protocol_test_case.rb index acdcae65c..ca05b1bad 100644 --- a/test/support/protocol_test_case.rb +++ b/test/support/protocol_test_case.rb @@ -338,8 +338,7 @@ def execute_dap_scenario scenario is_assertion_failure = true raise e ensure - kill_remote_debuggee test_info - if test_info.failed_process && !is_assertion_failure + if kill_remote_debuggee(test_info) && !is_assertion_failure flunk create_protocol_message "Expected the debuggee program to finish" end # Because the debuggee may be terminated by executing the following operations, we need to run them after `kill_remote_debuggee` method. @@ -372,8 +371,7 @@ def execute_cdp_scenario_ scenario is_assertion_failure = true raise e ensure - kill_remote_debuggee test_info - if test_info.failed_process && !is_assertion_failure + if kill_remote_debuggee(test_info) && !is_assertion_failure flunk create_protocol_message "Expected the debuggee program to finish" end # Because the debuggee may be terminated by executing the following operations, we need to run them after `kill_remote_debuggee` method. diff --git a/test/support/test_case.rb b/test/support/test_case.rb index 976ad30fc..5b88b6a1e 100644 --- a/test/support/test_case.rb +++ b/test/support/test_case.rb @@ -122,17 +122,17 @@ def wait_pid pid, sec true end - def kill_safely pid, name, test_info - return if wait_pid pid, TIMEOUT_SEC - - test_info.failed_process = name + def kill_safely pid + return false if wait_pid pid, TIMEOUT_SEC Process.kill :TERM, pid - return if wait_pid pid, 0.2 + return true if wait_pid pid, 0.2 Process.kill :KILL, pid Process.waitpid(pid) + true rescue Errno::EPERM, Errno::ESRCH + true end def check_error(error, test_info) @@ -142,13 +142,14 @@ def check_error(error, test_info) end def kill_remote_debuggee test_info - return unless r = test_info.remote_info + return false unless r = test_info.remote_info - kill_safely r.pid, :remote, test_info + force_killed = kill_safely r.pid r.reader_thread.kill # Because the debuggee may be terminated by executing the following operations, we need to run them after `kill_safely` method. r.r.close r.w.close + force_killed end def setup_remote_debuggee(cmd) From 64732f602643384326e361ef2382b94fd83caf66 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Thu, 9 Mar 2023 22:22:17 +0800 Subject: [PATCH 8/8] Remove unused failed_process attribute --- test/support/test_case.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/support/test_case.rb b/test/support/test_case.rb index 5b88b6a1e..5fff5a612 100644 --- a/test/support/test_case.rb +++ b/test/support/test_case.rb @@ -14,7 +14,7 @@ module DEBUGGER__ class TestCase < Test::Unit::TestCase TestInfo = Struct.new(:queue, :mode, :prompt_pattern, :remote_info, - :backlog, :last_backlog, :internal_info, :failed_process) + :backlog, :last_backlog, :internal_info) RemoteInfo = Struct.new(:r, :w, :pid, :sock_path, :port, :reader_thread, :debuggee_backlog)