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

Add explanations about how to generate tests in DAP #513

Merged
merged 3 commits into from
Feb 5, 2022

Conversation

ono-max
Copy link
Collaborator

@ono-max ono-max commented Feb 2, 2022

No description provided.

@ono-max
Copy link
Collaborator Author

ono-max commented Feb 2, 2022

@st0012

Feel free to tell me if this explanation is not enough.

@ono-max ono-max changed the title Add explanations about how to generate tests Add explanations about how to generate tests in DAP Feb 2, 2022
CONTRIBUTING.md Outdated Show resolved Hide resolved
ono-max and others added 2 commits February 2, 2022 21:39
Co-authored-by: Olle Jonsson <olle.jonsson@gmail.com>
@st0012
Copy link
Member

st0012 commented Feb 2, 2022

@ono-max thanks for the instructions! I have a question though: how do I know what steps were taken from the original tests?

the written tests are low-level requests/responses and it's hard to reverse-engineer the original actions. for example, how do I know if the scope request here was done by the client automatically (for UI), or it's triggered by clicking something?

@ono-max
Copy link
Collaborator Author

ono-max commented Feb 2, 2022

for example, how do I know if the scope request here was done by the client automatically (for UI), or it's triggered by clicking something?

Well, if I want to know them, I run the command like $RUBY_DEBUG_DAP_SHOW_PROTOCOL=1 rdbg target.rb, then protocol messages will be outputted in the terminal. So, I compare the results of the tests with them, and I know them.

@st0012
Copy link
Member

st0012 commented Feb 3, 2022

I mean the test case itself doesn't tell me what actions were performed originally. I can try recording it several times and select the most similar result. But that's not efficient because any mis-click or forgotten step means a restart. Ideally, documenting all the steps inside the test case should help. However, I don't think that's a sustainable way.

I think we should have unit tests for individual commands. And they can be written with test helpers:

perform_dap_request "threads"
assert_dap_response {
  threads: [
    {
      id: 1,
      name: /#1 .*/
    }
  ]
}
perform_dap_request "stackTrace", {
  threadId: 1,
  startFrame: 0,
  levels: 20
}
assert_dap_response {
  stackFrames: [
    {
      name: "<main>",
      line: 1,
      column: 1,
      source: {
        name: /#{File.basename temp_file_path}/,
        path: /#{temp_file_path}/,
        sourceReference: nil
      },
      id: 1
    }
  ]
}

It'll be easy to add/update them, just like all the tests under test/debug/.

We'll still keep the recorded tests as integration tests, probably something like

PROGRAM = <<~RUBY
    # set breakpoints in line 20
    # continue
    # show backtrace
    # show info
    # quit
    class Foo
      def first_call
        second_call(20)
      end
    
      def second_call(num)
        third_call_with_block do |ten|
          forth_call(num, ten)
        end
      end
    
      def third_call_with_block(&block)
        @ivar1 = 10; @ivar2 = 20
    
        yield(10)
      end
    
      def forth_call(num1, num2)
        num1 + num2
      end
    end
RUBY

Then we can easily re-record them when needed according to the steps. And because there should be relatively few of them, maintaining them will be easier.

@ono-max
Copy link
Collaborator Author

ono-max commented Feb 3, 2022

> I think we should have unit tests for individual commands. And they can be written with test helpers:

You mean, we should prepare helper methods like dap_request(), or assert_dap_response()?

If it's your opinion, I have the following question.

* How can we find the response of threads in this case?

Sorry, never mind the above topics. I'll rewrite them later.

@ono-max
Copy link
Collaborator Author

ono-max commented Feb 4, 2022

First, I'd like to know your opinion in details.

Your opinion is

  • We should have unit tests.
  • We should use methods instead of hash.

Is that correct?

@st0012
Copy link
Member

st0012 commented Feb 4, 2022

  1. For individual commands, we should use unit tests. Having threads request and response in break tests isn't necessary and it increases maintenance effort. For example, if we change the response of scopes command, all of these tests will be affected:
test/dap/next_test.rb
111:            command: "scopes",
120:            command: "scopes",
125:              scopes: [
550:            command: "scopes",
559:            command: "scopes",
564:              scopes: [

test/dap/break_test.rb
111:            command: "scopes",
120:            command: "scopes",
125:              scopes: [
400:            command: "scopes",
409:            command: "scopes",
414:              scopes: [
579:            command: "scopes",
588:            command: "scopes",
593:              scopes: [
739:            command: "scopes",
748:            command: "scopes",
753:              scopes: [

test/dap/boot_config_test.rb
185:            command: "scopes",
194:            command: "scopes",
199:              scopes: [

test/dap/detach_test.rb
114:            command: "scopes",
123:            command: "scopes",
128:              scopes: [
549:            command: "scopes",
558:            command: "scopes",
563:              scopes: [

test/dap/step_test.rb
111:            command: "scopes",
120:            command: "scopes",
125:              scopes: [
689:            command: "scopes",
698:            command: "scopes",
703:              scopes: [
826:            command: "scopes",
835:            command: "scopes",
840:              scopes: [

test/dap/finish_test.rb
111:            command: "scopes",
120:            command: "scopes",
125:              scopes: [
318:            command: "scopes",
327:            command: "scopes",
332:              scopes: [
484:            command: "scopes",
493:            command: "scopes",
498:              scopes: [

test/dap/step_back_test.rb
113:            command: "scopes",
122:            command: "scopes",
127:              scopes: [
404:            command: "scopes",
413:            command: "scopes",
418:              scopes: [
559:            command: "scopes",
568:            command: "scopes",
573:              scopes: [
725:            command: "scopes",
734:            command: "scopes",
739:              scopes: [
865:            command: "scopes",
874:            command: "scopes",
879:              scopes: [

test/dap/hover_test.rb
107:            command: "scopes",
116:            command: "scopes",
121:              scopes: [
332:            command: "scopes",
341:            command: "scopes",
346:              scopes: [
971:            command: "scopes",
980:            command: "scopes",
985:              scopes: [
1164:            command: "scopes",
1173:            command: "scopes",
1178:              scopes: [

And because the recording approach almost always generate certain commands, like scopes or threads, I don't think it's suitable for unit testing. Therefore, I hope we can have dap_request and assert_dap_response for those unit tests.

  1. For integration testing, the recording approach is a great option. But currently it's hard to understand what operations were done in a test case (e.g. is the threads request made from the VSCode automatically, or it's generated by a user action).
    I know we can figure it out by performing a similar operation with RUBY_DEBUG_DAP_SHOW_PROTOCOL. But it's not user-friendly and requires much manual work.
    Since it's unlikely that we can record what the user does on an UI level (e.g. expanded a local variable). So I'd hope there is a standard to let test-author writing down what actions he/she performed to generate the test.

  2. I think in the long-term, protocol-level testing (the current request/response tests) should mainly be used for unit testing. And integration testing should use something like vscode-extension-tester (here's an article for it) that performs real end-to-end testing from the VSCode. It'll be similar to how many developers tests frontend with selenium.
    However, an obvious downside of this is the need to write javascript 😂

@ono-max
Copy link
Collaborator Author

ono-max commented Feb 5, 2022

Thank you for explaining to me.
Let me rethink about it.

@ono-max
Copy link
Collaborator Author

ono-max commented Feb 5, 2022

@st0012

protocol-level testing (the current request/response tests) should mainly be used for unit testing

Because we have already wrote some tests? From 1 and 2, I thought you want to change from protocol-level testing to using some methods such as dap_request and assert_dap_response.

@st0012
Copy link
Member

st0012 commented Feb 5, 2022

Because we have already wrote some tests?

I'd be happy to rewrite them into simpler ones, like for stepping tests we only test related endpoints and wihout threads or scopes ones.

I thought you want to change from protocol-level testing to using some methods such as dap_request and assert_dap_response.

that'll be an improvement for sure and perhaps we can start from there. but I think the clear separation for unit/integration testing is more important.

for example, break_test.rb shouldn't contain dap_request("scopes") as it's irrelevant to the endpoint we want to test. and the recording approach can't clearly exclude those requests, so I think we shouldn't use it in command-specific tests like break_test.rb.

@ko1 ko1 merged commit 75af37e into ruby:master Feb 5, 2022
@ko1
Copy link
Collaborator

ko1 commented Feb 5, 2022

please continue the discussion on another ticket about the test data format.

@ko1
Copy link
Collaborator

ko1 commented Feb 5, 2022

or you can continue here.

@ono-max
Copy link
Collaborator Author

ono-max commented Feb 6, 2022

Ahh, I got your points. I'll rethink it.

@st0012
Copy link
Member

st0012 commented Feb 13, 2022

@ono-max if you think it's ok to convert most of the command-specific tests into unit-tests, we can do that as follows:

  1. Open a PR to add dap_request and assert_dap_response helpers.
  2. Start rewriting simpler tests like detach_test or hover_test.
    • By rewriting I mean using the new helpers + don't include unrelated request/response.
  3. I think the current step_test and step_back_test are complicated enough to be used as integration tests.
  4. Convert the rest of the tests into unit-tests.

@ono-max
Copy link
Collaborator Author

ono-max commented Feb 13, 2022

I create a prototype to think about it. Could you give me some more time? Thanks.

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

4 participants