Skip to content
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

Test for remote debugger #76

Merged
merged 4 commits into from
Jul 1, 2021
Merged

Conversation

ono-max
Copy link
Collaborator

@ono-max ono-max commented Jun 9, 2021

I implement the test framework for remote debugger.

  • Assign the argument remote: false in debug_code method when using require "debug/run" or DEBUGGER__.console.
  • Test for remote and local are in each method by one. That's why if first test(local) failed, second test(remote) won't work.
    • Test for local test will be executed first, and Test for remote(UNIX domain socket mode and TCP/IP mode) will be executed next.

test/support/utils.rb Outdated Show resolved Hide resolved
test/support/utils.rb Outdated Show resolved Hide resolved
@ono-max ono-max force-pushed the test-for-remote-debugger branch 4 times, most recently from 2aaf758 to 8184d3a Compare June 10, 2021 13:35
@st0012
Copy link
Member

st0012 commented Jun 10, 2021

If we can further improve the current test framework and not relying on the internal info for testing (e.g. run tests by purely recording input/output), we can avoid patching either the server nor client for remote testing. This approach has some benefits:

  • We don't need to maintain any extra layers/patches for the testing mode so the lib codebase would be clean.
  • Remote debugging (TCP/IP) will be a lot easier to implement because the extra patches won't be not needed.
  • It'll be able to test irb integration without extra patches on it.

@ono-max @ko1 wdyt?

@ono-max
Copy link
Collaborator Author

ono-max commented Jun 10, 2021

@st0012
We'll discuss it today. Just a moment, please.
Thank you.

@ono-max ono-max force-pushed the test-for-remote-debugger branch 3 times, most recently from e695a6e to 5ad682f Compare June 11, 2021 01:02
r, w, server_pid = PTY.spawn("#{RUBY} #{boot_options} #{temp_file_path}")
r.close
sleep(0.1)
cmd = "exe/rdbg -A"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not use this path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ono-max
Copy link
Collaborator Author

ono-max commented Jun 13, 2021

@st0012
I'm sorry for the late reply.

What you're trying to say is that "we don't want to create extra patches", right?
I can output the internal info without extra patches such as #84, and I don't need to write codes for it.

@ono-max ono-max force-pushed the test-for-remote-debugger branch 9 times, most recently from de725e0 to 62f4bce Compare June 19, 2021 04:26
@ono-max ono-max marked this pull request as ready for review June 19, 2021 04:29
@ono-max ono-max changed the title [WIP]test for remote debugger Test for remote debugger Jun 19, 2021
@ko1
Copy link
Collaborator

ko1 commented Jun 22, 2021

could you introduce local only mode for fast debug?

@ko1
Copy link
Collaborator

ko1 commented Jun 22, 2021

Also, now UNIX domain socket mode is supported. Could you add an option to test on TCP/IP mode?

inject_lib_to_load_path
ENV['RUBY_DEBUG_USE_COLORIZE'] = "false"
ENV['RUBY_DEBUG_TEST_MODE'] = 'true'

timeout_sec = (ENV['RUBY_DEBUG_TIMEOUT_SEC'] || 10).to_i

PTY.spawn("#{RUBY} #{boot_options} #{temp_file_path}") do |read, write, pid|
if boot_options == "-r debug/open"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has bad smell.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified it.

@ono-max ono-max force-pushed the test-for-remote-debugger branch 2 times, most recently from 0bb8d2b to 70cf2a6 Compare June 24, 2021 02:16
@ono-max ono-max requested a review from ko1 June 24, 2021 03:16
@ono-max
Copy link
Collaborator Author

ono-max commented Jun 24, 2021

could you introduce local only mode for fast debug?

I did it by an environmental variable.
2beeb4f#diff-ee14bf65f14927f7778a246f90c5cec0c581be3e2658c2a8a3726c59db5be3f9R40

Also, now UNIX domain socket mode is supported. Could you add an option to test on TCP/IP mode?

I supported it.
2beeb4f#diff-ee14bf65f14927f7778a246f90c5cec0c581be3e2658c2a8a3726c59db5be3f9R51-R52

@ono-max
Copy link
Collaborator Author

ono-max commented Jun 24, 2021

@st0012
You haven't replied me. Can I resolve conversation with you?

@st0012
Copy link
Member

st0012 commented Jun 25, 2021

@ono-max sorry I missed the notifications. given the scope of this PR, I'll leave the rest of the review to @ko1, so you can resolve the comments now 🙂

cmd = "#{__dir__}/../../exe/rdbg -A"
new_child_process("#{RUBY} #{boot_options} #{temp_file_path}")
create_pseudo_terminal(cmd, repl_prompt)
cmd = "#{__dir__}/../../exe/rdbg -A 12345"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PORT should be given by an option, maybe RUBY_DEBUG_TEST_PORT or something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inject_lib_to_load_path

if remote && ENV['RUBY_DEBUG_LOCAL_ONLY_MODE'] == 'false'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RUBY_DEBUG_TEST prefix is better to make clear the purpose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ono-max ono-max force-pushed the test-for-remote-debugger branch 3 times, most recently from 32432b2 to e91ef1e Compare June 28, 2021 22:15
@ono-max ono-max requested a review from ko1 June 28, 2021 22:25
@ono-max
Copy link
Collaborator Author

ono-max commented Jul 1, 2021

@st0012
I modified codes based on your comments. Are they ok for you?

@st0012
Copy link
Member

st0012 commented Jul 1, 2021

@ono-max yes. thanks for the quick update 👍

@ko1 ko1 merged commit 8dfe757 into ruby:master Jul 1, 2021
@ono-max ono-max deleted the test-for-remote-debugger branch November 2, 2021 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants