Add assert locals to protocol tests

[st0012]

No description provided.

[st0012]

cc @ono-max

[st0012]

And since we now also cover local variables, I think the "raw" tests can be deleted?

[ono-max]

I prefer "assert_local_result expected, variable: nil" to align with other methods.

[ono-max]

I prefer "assert_assert_local_result expected, variable: " to align with others.

[st0012]

@ono-max how about having both? assert_local_result value: val, name: name, type: type and assert_local_results(array)

[ono-max]

1. In the testing APIs, I want to minimize the number of APIs because it's simple for users to write tests.

2. As I said before, I prefer assert_local_result expected, variable: nil or assert_local_result expected, name: nil to align with others.

[st0012]

We don't need to write types of values because they can be inferred from expected

That's interesting, how does the framework now if we expect nil or "nil" from CDP response? I think it'll return "nil" for both cases with different types?

[ono-max]

You might want to read https://github.com/ruby/debug/blob/master/test/support/protocol_utils.rb#L414.

[st0012]

The current value assertion imo isn't practical in the long term. Not passing type information means we need to replicate the actual value in the test framework, like

debug/test/protocol/hover_test.rb

Lines 82 to 88 in 12c5ac3

	assert_hover_resultObject.const_set('Abc',Module.new),expression: 'Abc'
	assert_hover_resultAbc.const_set('Def123',Class.new),expression: 'Abc::Def123'
	assert_hover_resultAbc::Def123.const_set('Ghi',Class.new),expression: 'Abc::Def123::Ghi'
	assert_hover_resultAbc::Def123::Ghi,expression: 'Abc::Def123::Ghi.new'
	assert_hover_resultAbc,expression: '::Abc.foo'
	assert_hover_resultAbc::Def123,expression: '::Abc::Def123'
	assert_hover_resultAbc::Def123,expression: '::Abc::Def123.bar'

This means that different test cases are not isolated and that'll make future tests harder to write (because we need to know what constants defined by other tests) and harder to debug.

[ono-max]

Right. I agree with your opinion.
Let me rethink about assert_hover_result, first. Then, I'll be back.
Thx.

[st0012]

@ono-max I don't like the idea that you always own the CDP/DAP test framework and everything to be added needs to be approved by you. Why can't we have this helper first and then you can refactor it when you have a clear idea?

You didn't get the framework right the first time with #547, right? It actually broke, which is normal in software development. And you later updated it and it works for all of us now. So why can't I implement the assertion helper this way first and need to wait for your implementation? It's not public API and can always be changed.

[st0012]

@ono-max Simply put: You can and should rethink about assert_hover_result. But unless there are obvious flaws in the assert_locals_result helper, I don't know why we need to block its merge when you doing that.

If you later want to change it to assert_local_result expected, name:, type:, we can discuss that.

[ono-max]

I don't like the idea that you always own the CDP/DAP test framework and everything to be added needs to be approved by you.

In CDP/DAP tests, @ko1-san doesn't review it, and when PRs are created about changes to the framework, he asks me to review them. That's why I think that I am responsible for them and need to approve them.

---

Although changes to the framework don't affect debugger users, it affects contributors who write tests. Because I want to minimize the impact, I think that I need to decide assert_hover_result first. Then, assert_locals_result can align with them.

[st0012]

Although changes to the framework don't affect debugger users, it affects contributors who write tests.

There could be contributors affected by the changes, which is an assumption. But what we surely know now is that we can use assert_locals_result helper right now, in some tests. And plus with some other new test helpers, we can remove all raw tests. Why can't we prioritize that?

Another thing is that, we as software developers don't make things right at the first, 2nd, or even the 3rd try. Trying to make things perfect and align the later implementation is an illusion. And more importantly, this shouldn't be a reason to block other progress.

You used the whole #547 PR to implement the framework and helpers, but it didn't workout at all after it's merged. And you didn't even spotted it. No other contributors spotted it either. I spotted it and raised it to you. Because I'm the one who use this framework. You also didn't spot the flaw in the current design, I spotted it, just in the above comments. Because again, I'm the contributor impacted by those issues.

So can you stop assuming there's other contributors that may be affected by this, while I, who does write these tests, telling you I'm already affected by the lack of type information and assert_locals_result helper.

I know and understand that you want to make this framework great. And you did make it great over the time, with mistakes and discussions, like #513. But your responsibility to the framework shouldn't become an excuse to hold it and not letting other people touching it, or ask them to develop with your own pace. This is not your personal project.

[ono-max]

Thank you for explaining to me.
Ok, I accept this PR.

[st0012]

Thank you.
Now, my suggestion to methods like assert_hover_result is to let all of them take type: argument and make the tests isolated again.

And perhaps later we can set rules to infer some value types automatically, like:

• 1 for integer
• "\"foobar\"" for string
• Foo for class/module (in most cases we don't need to distinguish them)

So assert_hover_result 1, expression: "i" doesn't need type: argument.

But for matching Ruby objects (e.g. instance of Post), we can match them with type: assert_hover_result expression: "p", type: "Post"

[ono-max]

Oh, I noticed that this is incorrect. We can't use :type as Ruby class.
I'll fix it later.

[st0012]

That'd be great, thank you.

[ono-max]

Could you add assert_locals_result to CONTRIBUTING.md?

[st0012]

@ono-max 👍 added

[ono-max]

I think that we should support Regexp because we can't use assert_locals_result in the following case:

class Foo
def initialize a, b
@a = a
@b = b
end
end
a = Foo.new 1, 2
p a # Set a breakpoint here and type `info locals`.

a will be something like a = #<Foo:0x0000000104b37608 @a=1, @b=2>.

[st0012]

How about { name: "a", type: "Foo", value: { "@a" => 1, "@b" => 2 } }?
And in the cases we don't need to validate the ivars, just { name: "a", type: "Foo"}

[ono-max]

For me, it's easier to write a regular expression and read it.

[st0012]

Sure, so { name: "a", type: "Foo", value: /@a=1, @b=2/ }?

[st0012]

@ono-max I think this is good to merge?

[ono-max]

Almost.

You forgot to change my commit suggestions like #559 (comment) or #559 (comment). Maybe, when you rebase this PR, they are removed.

[st0012]

Ah right, they must be left during the force push. I've re-added them.

[ono-max]

Great!
LGTM