Update protocol tests #720

st0012 · 2022-07-16T13:35:43Z

Add termination tests
Test CDP and DAP's disconnection separately because DAP has more options to cover
Remove obsolete detaching tests because of ^
Remove unnecessary restart tests (they actually doesn't cover the feature)

This PR doesn't fix the flaky tests because send_dap_request still waits for the response of disconnect request. But that request is not deterministic as the debugger can execute the kill! command (and exit) before returning the response. I hope #721 can address the issue.

ono-max

I left some comments.

test/support/protocol_test_case.rb

test/protocol/disconnect_dap_test.rb

CONTRIBUTING.md

test/protocol/disconnect_dap_test.rb

ono-max · 2022-11-06T12:19:54Z

test/support/protocol_test_case.rb

+      end
+      sleep 0.001 while @reader_thread.status != 'sleep'
+      @reader_thread.run
+      INITIALIZE_CDP_MSGS.each{|msg| send(**msg)}


Please add the following lines here.
https://github.com/ruby/debug/blob/master/test/support/protocol_test_case.rb#L415-L416

The reason for adding this here is as follows:

call attach_to_cdp_server directory.

call something that uses @crt_frames

@crt_frames is not the latest frame because it may be changed every time 'Debugger.paused' is called.

I think Debugger.paused is only returned at the first connection?

Loaded suite test/protocol/disconnect_cdp_test Started start initial attach {:id=>2, :result=>{}} {:method=>"Runtime.executionContextCreated", :params=>{:context=>{:id=>"eae712537be02fa944b64faad339a2ab", :origin=>"http://127.0.0.1:63585", :name=>""}}} {:id=>3, :result=>{}} {:id=>4, :result=>{}} {:id=>8, :result=>{}} {:method=>"Debugger.scriptParsed", :params=>{:scriptId=>"1", :url=>"/private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug-20221106-84811-rdzgps.rb", :startLine=>0, :startColumn=>0, :endLine=>12, :endColumn=>0, :executionContextId=>1, :hash=>"2195685978487654025"}} {:method=>"Debugger.paused", :params=>{:reason=>"other", :callFrames=>[{:callFrameId=>"0fb3aefabd50b783487268956071f58c", :functionName=>"<main>", :functionLocation=>{:lineNumber=>0, :scriptId=>"1"}, :location=>{:lineNumber=>0, :scriptId=>"1"}, :url=>"/private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug-20221106-84811-rdz gps.rb", :scopeChain=>[{:type=>"local", :object=>{:type=>"object", :objectId=>"0.449851775437477"}}, {:type=>"script", :object=>{:type=>"object", :objectId=>"0.07042992169684925"}}, {:type=>"global", :object=>{:type=>"object", :objectId=>"0.7769977982489024"}}], :this=>{:type=>"object"}}]}} end initial attach disconnected start second attach {:id=>2, :result=>{}} {:method=>"Runtime.executionContextCreated", :params=>{:context=>{:id=>"bfb3534d760e52e0db4d88a84ea031a4", :origin=>"http://127.0.0.1:63585", :name=>""}}} {:id=>3, :result=>{}} {:id=>4, :result=>{}} {:id=>8, :result=>{}} F ============================================================================================================================================================================================================================================================================================================================================= Failure: test_closing_cdp_connection_doesnt_kill_the_debuggee(DEBUGGER__::DisconnectCDPTest):

That's why I didn't move those 2 lines as it'll break reconnected cases like test/protocol/disconnect_cdp_test

Oh, seriously? Thank you for checking it. I'll also check it later.

I just checked the behavior, and you are right. Also, because I noticed that this behavior is a bug, I need to fix it, too.

I can notice this bug thanks to your reporting. Thank you.

If it's OK with you, could you write comments somewhere as follows?

TODO: Support reattaching in Chrome DevTools

Sure. But should I also remove the reattaching part from test/protocol/disconnect_cdp_test.rb as it's actually not working?

But should I also remove the reattaching part from test/protocol/disconnect_cdp_test.rb as it's actually not working?

If you can, yes.

I found that we still need to reattach to the CDP server in disconnection test because we need to terminate it. But I added the comment right above the attaching call so others can understand that's not how actual reattaching works.

ono-max · 2022-11-13T11:27:05Z

I think that this PR looks good to me 👍

Thank you for your great work!

ko1 · 2022-11-15T03:02:28Z

Remove unnecessary restart tests (feature not supported)

Please remain this test because user can push the restart button on the VSCode.
And I'll merge this PR.

st0012 · 2022-11-15T10:07:23Z

@ko1 The test actually doesn't test restart but just termination:

debug/test/protocol/restart_test.rb

Lines 12 to 16 in 1ecb014

    
           def test_restart 
        
             run_protocol_scenario PROGRAM do 
        
               req_terminate_debuggee 
        
             end 
        
           end

ono-max · 2022-11-15T10:58:10Z

Sorry, using req_terminate_debuggee in restart test was my mistake. Here is my suggestion to test restart meaningfully:

Define the following method such as follows:

def req_restart
      case get_target_ui
      when 'vscode'
        send_dap_request 'terminate', restart: true
      end
    end

Use it in restart test.

WDYT?

st0012 · 2022-11-15T11:16:06Z

@ono-max If we currently only support vscode for restart, I'd prefer not adding req_restart yet. Otherwise other devs need to check its definition to find out CDP actually is not supported by it.

ono-max · 2022-11-15T11:19:16Z

Ok, so how about this one? we define req_restart method as you did here. And we can add a prefix like dap_restart_test.rb.

https://github.com/ruby/debug/pull/720/files#diff-172faac32ddd51787eb1039d2109dc1a698cae3d3753c8140c36a3ad9aeb9940R42

st0012 · 2022-11-15T11:44:15Z

Please remain this test because user can push the restart button on the VSCode.

After some testing, I think DAP's restart actually doesn't work.

DAP.restart.not.working.mov

The last DAP request is:

#11078:[>] {"command":"disconnect","arguments":{"restart":true,"terminateDebuggee":true},"type":"request","seq":31}
#11078:[<] {"type":"response","command":"disconnect","request_seq":31,"success":true,"message":"Success","seq":37}

Based on the specification, if we're not supporting the restart request yet (commented out), we need to emulate the restarting behavior by starting a new debuggee manually in the server.

And we can see that DAP server currently doesn't handle the restart argument:

debug/lib/debug/server_dap.rb

Lines 354 to 371 in 1ecb014

    
           when 'disconnect' 
        
             terminate = args.fetch("terminateDebuggee", false) 
        
             SESSION.clear_all_breakpoints 
        
             send_response req 
        
             if SESSION.in_subsession? 
        
               if terminate 
        
                 @q_msg << 'kill!' 
        
               else 
        
                 @q_msg << 'continue' 
        
               end 
        
             else 
        
               if terminate 
        
                 @q_msg << 'kill!' 
        
                 pause 
        
               end 
        
             end

So it appears that we actually don't support restart yet?

If it's confirmed, I can convert this comment into an issue for the feature.

ko1 · 2022-11-17T17:34:46Z

Now "launch" mode "Restart Debug" command (from menu) seems working.
On "attach" mode it doesn't (DA support is required).

The spec https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Disconnect doesn't say anything I think.

  /**
   * A value of true indicates that this `disconnect` request is part of a
   * restart sequence.
   */
  restart?: boolean;

st0012 · 2022-11-18T17:58:17Z

Now "launch" mode "Restart Debug" command (from menu) seems working.

Yes but we can't cover this case atm because we can only test attach mode in protocol tests.

This means the current test tries to test a feature we're not supporting yet. And it is passing because it actually just test disconnection, which we already covered in other tests. That's why I want to delete that test.

ko1 · 2022-11-22T02:27:29Z

ok I merge it.

st0012 force-pushed the update-protocol-tests branch from ae686fa to fc42542 Compare July 16, 2022 14:01

st0012 force-pushed the update-protocol-tests branch from fc42542 to 7de6f3b Compare August 10, 2022 15:35

ono-max reviewed Sep 16, 2022

View reviewed changes

test/support/protocol_test_case.rb Outdated Show resolved Hide resolved

test/support/protocol_test_case.rb Show resolved Hide resolved

test/protocol/disconnect_dap_test.rb Show resolved Hide resolved

test/protocol/disconnect_dap_test.rb Outdated Show resolved Hide resolved

st0012 force-pushed the update-protocol-tests branch 2 times, most recently from 8b1d13f to 3e909f2 Compare October 26, 2022 10:59

ono-max reviewed Oct 29, 2022

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

ono-max reviewed Oct 29, 2022

View reviewed changes

test/protocol/disconnect_dap_test.rb Outdated Show resolved Hide resolved

ono-max reviewed Oct 29, 2022

View reviewed changes

test/protocol/disconnect_dap_test.rb Show resolved Hide resolved

st0012 force-pushed the update-protocol-tests branch 3 times, most recently from 3229bac to 19c98f8 Compare November 3, 2022 11:20

st0012 requested a review from ono-max November 6, 2022 11:23

ono-max reviewed Nov 6, 2022

View reviewed changes

st0012 force-pushed the update-protocol-tests branch from 19c98f8 to 8559e3c Compare November 13, 2022 11:47

st0012 force-pushed the update-protocol-tests branch from 8559e3c to 7777b48 Compare November 15, 2022 11:47

st0012 added 3 commits November 18, 2022 18:23

Add request-based termination/disconnection protocol tests

1265815

Remove incorrect restart tests

abaa14e

Remove unnecessary detach tests

6e7ad33

st0012 force-pushed the update-protocol-tests branch from 7777b48 to 6e7ad33 Compare November 18, 2022 18:25

ko1 merged commit 1c8d84a into ruby:master Nov 22, 2022

st0012 deleted the update-protocol-tests branch November 22, 2022 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update protocol tests #720

Update protocol tests #720

st0012 commented Jul 16, 2022 •

edited

Loading

ono-max left a comment

ono-max Nov 6, 2022

ono-max Nov 6, 2022

st0012 Nov 6, 2022 •

edited

Loading

ono-max Nov 7, 2022

ono-max Nov 13, 2022 •

edited

Loading

ono-max Nov 13, 2022 •

edited

Loading

st0012 Nov 13, 2022

ono-max Nov 13, 2022

st0012 Nov 13, 2022

ono-max Nov 13, 2022

ono-max commented Nov 13, 2022

ko1 commented Nov 15, 2022

st0012 commented Nov 15, 2022

ono-max commented Nov 15, 2022 •

edited

Loading

st0012 commented Nov 15, 2022

ono-max commented Nov 15, 2022 •

edited

Loading

st0012 commented Nov 15, 2022 •

edited

Loading

ko1 commented Nov 17, 2022 •

edited

Loading

st0012 commented Nov 18, 2022 •

edited

Loading

ko1 commented Nov 22, 2022

Update protocol tests #720

Update protocol tests #720

Conversation

st0012 commented Jul 16, 2022 • edited Loading

ono-max left a comment

Choose a reason for hiding this comment

ono-max Nov 6, 2022

Choose a reason for hiding this comment

ono-max Nov 6, 2022

Choose a reason for hiding this comment

st0012 Nov 6, 2022 • edited Loading

Choose a reason for hiding this comment

ono-max Nov 7, 2022

Choose a reason for hiding this comment

ono-max Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

ono-max Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

st0012 Nov 13, 2022

Choose a reason for hiding this comment

ono-max Nov 13, 2022

Choose a reason for hiding this comment

st0012 Nov 13, 2022

Choose a reason for hiding this comment

ono-max Nov 13, 2022

Choose a reason for hiding this comment

ono-max commented Nov 13, 2022

ko1 commented Nov 15, 2022

st0012 commented Nov 15, 2022

ono-max commented Nov 15, 2022 • edited Loading

st0012 commented Nov 15, 2022

ono-max commented Nov 15, 2022 • edited Loading

st0012 commented Nov 15, 2022 • edited Loading

ko1 commented Nov 17, 2022 • edited Loading

st0012 commented Nov 18, 2022 • edited Loading

ko1 commented Nov 22, 2022

st0012 commented Jul 16, 2022 •

edited

Loading

st0012 Nov 6, 2022 •

edited

Loading

ono-max Nov 13, 2022 •

edited

Loading

ono-max Nov 13, 2022 •

edited

Loading

ono-max commented Nov 15, 2022 •

edited

Loading

ono-max commented Nov 15, 2022 •

edited

Loading

st0012 commented Nov 15, 2022 •

edited

Loading

ko1 commented Nov 17, 2022 •

edited

Loading

st0012 commented Nov 18, 2022 •

edited

Loading