Skip to content

Add assert_threads_result helper and threads test #607

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

Merged
merged 4 commits into from
Apr 17, 2022

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Mar 30, 2022

Some notes:

  • Ids don't always start from 0 and increment by 1, as shown in the below example. So asserting ids is pointless imo.
  • Threads request is only supported in DAP server.
  • Example failure message:
result:
{
  "type": "response",
  "command": "threads",
  "request_seq": 8,
  "success": true,
  "message": "Success",
  "body": {
    "threads": [
      {
        "id": 1,
        "name": "#1 /var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug-20220411-21868-x0t9a2.rb:6:in `<main>'"
      },
      {
        "id": 4,
        "name": "#4 /var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug-20220411-21868-x0t9a2.rb:2:in `block in foo'"
      }
    ]
  },
  "seq": 9
}.
Expect all thread names to be matched. Unmatched threads:
<[]> expected but was
<["#1 /var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug-20220411-21868-x0t9a2.rb:6:in `<main>'"]>

diff:
? ["#1 /var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug-20220411-21868-x0t9a2.rb:6:in `<main>'"]

@st0012 st0012 force-pushed the add-threads-protocol-test branch from abfc353 to 63e483f Compare March 30, 2022 21:47
@ono-max
Copy link
Member

ono-max commented Apr 2, 2022

Could you update CONTRIBUTING.md?

@st0012
Copy link
Member Author

st0012 commented Apr 2, 2022

Sure. But speaking of documentation, do you think we can do that in Ruby comments? Like test-unit does. And in the contribution guideline we just link to the file.

I think it fits the convention better and it's easier for writers of methods (like me) to remember too 😛

@ono-max
Copy link
Member

ono-max commented Apr 2, 2022

We can write how to use API in the comments and CONTRIBUTING.md. I think that writing the explanation in CONTRIBUTING.md is good because they are well organized.

@st0012
Copy link
Member Author

st0012 commented Apr 2, 2022

I don't think having documentation in 2 places is a good idea. It's the same reason we only write command intro in session.rb and generate it in README.md, instead of having it in both places.

@st0012 st0012 force-pushed the add-threads-protocol-test branch from 6a0cc9c to f66966f Compare April 2, 2022 14:38
@ono-max
Copy link
Member

ono-max commented Apr 2, 2022

Well, I see. Or, we can create rdoc page from the comment?

@st0012
Copy link
Member Author

st0012 commented Apr 2, 2022

Yes. In that case, we'll need to write the comments follow yard's specification.

@st0012 st0012 force-pushed the add-threads-protocol-test branch 2 times, most recently from c54ba10 to 9b7333d Compare April 5, 2022 08:26
@st0012
Copy link
Member Author

st0012 commented Apr 5, 2022

@ono-max I've updated the contribution guideline for now. can you check it? thx

@ono-max
Copy link
Member

ono-max commented Apr 6, 2022

Nice work! LGTM

@st0012 st0012 force-pushed the add-threads-protocol-test branch 2 times, most recently from 715079b to 6430f40 Compare April 11, 2022 20:10
@ono-max
Copy link
Member

ono-max commented Apr 12, 2022

You missed https://github.com/ruby/debug/pull/607/files#r843338555. Could you update it?


expected_names.each do |expected|
thread_names.reject! do |name|
name.match?(expected)
Copy link
Member

@ono-max ono-max Apr 12, 2022

Choose a reason for hiding this comment

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

We can use assert_match method in this case, could you tell me the reason why you implemented so?

Copy link
Member Author

@st0012 st0012 Apr 12, 2022

Choose a reason for hiding this comment

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

Why do you think we can use it here? Remember, the order of threads info is not guaranteed. So we don't know which one will match.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's right. My bad 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries 😉
Do other changes look good to you?

@st0012 st0012 force-pushed the add-threads-protocol-test branch from 6430f40 to 8b9a85e Compare April 12, 2022 08:09
end

failure_msg = FailureMessage.new{create_protocol_message "result:\n#{JSON.pretty_generate res}.\nExpect all thread names to be matched. Unmatched threads:"}
assert_equal [], thread_names, failure_msg
Copy link
Member

Choose a reason for hiding this comment

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

Can we use assert_empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to use assert_equal because it provides better highlighting in the failure messages:

截圖 2022-04-17 10 25 01

assert_empty

截圖 2022-04-17 10 25 15

I guess it's because assert_empty doesn't care about individual items in the array, but assert_equal does. So it provides a more detailed information.

@ko1 ko1 merged commit 98b2d31 into ruby:master Apr 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants