service/dap: avoid hardcoded []byte slices in DAP tests #1881
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aarzilli
approved these changes
Feb 21, 2020
service/dap/daptest/client.go
Outdated
| } | ||
|
|
||
| func (c *Client) ExpectContinueResponse(t *testing.T) *dap.ContinueResponse { | ||
| m, err := dap.ReadProtocolMessage(c.reader) |
There was a problem hiding this comment.
I think you would be better served by defining a helper function:
func mustReadProtocolMessage(t *testing.T, c *Client) dap.Message {
m, err := dap.ReadProtocolMessage(c.reader)
if err != nil {
t.Error(err)
}
return m
}
then you wouldn't even need these, you could just mustReadProtocolMessage(t, c).(*dap.ConcreteResponseType)
There was a problem hiding this comment.
Thanks for the suggestion. Just to make sure I understand precisely, so you mean I should use (1) this helper function to simplify the Expect* methods, or (2) replace all calls to Expect* with this?
E.g., replace:
contResp := client.ExpectContinueResponse(t)
With:
contResp := mustReadProtocolMessage(t, client).(*dap.ContinueResponse)
The tradeoff, IMHO is:
- The original has cleaner call sites in tests, so the tests themselves are somewhat easier to read.
- We have some custom logic in some
Expect*methods (e.g.ExpectInitializeResponse) and may add more such logic in the future. Removing the helpers means this logic needs to be duplicated at call sites, or have specific helpers just for relevant methods.
On the other hand, your proposal helps remove a bunch of repetitive methods, which is an advantage.
A middle way is (1), replace:
func (c *Client) ExpectContinueResponse(t *testing.T) *dap.ContinueResponse {
m, err := dap.ReadProtocolMessage(c.reader)
if err != nil {
t.Error(err)
}
return m.(*dap.ContinueResponse)
}
By:
func (c *Client) ExpectContinueResponse(t *testing.T) *dap.ContinueResponse {
return c.mustReadProtocolMessage().(*dap.ContinueResponse)
}
This keeps all the Expect* methods but makes them one-liners, except those where extra logic is needed. The call sites in tests remain short and readable, but the amount of repetitive code is reduced.
WDYT?
There was a problem hiding this comment.
I was thinking just:
contResp := mustReadProtocolMessage(t, client).(*dap.ContinueResponse)
but if you think you'll need to add extra logic then keeping the Expect methods makes sense.
There was a problem hiding this comment.
I like the idea of keeping the Expect methods, but factoring out all the common parts into a mustRead helper.
There was a problem hiding this comment.
Except for consistency it should be called ExpectProtocolMessage or the other one should be called MustReadFooRequest.
polinasok
reviewed
Feb 21, 2020
service/dap/daptest/client.go
Outdated
| // ReadBaseMessage reads and returns a json-encoded DAP message. | ||
| func (c *Client) ReadBaseMessage() ([]byte, error) { | ||
| message, err := dap.ReadBaseMessage(c.reader) | ||
| func (c *Client) ExpectDisconnectResponse(t *testing.T) *dap.DisconnectResponse { |
There was a problem hiding this comment.
Should these be calling t.Helper()?
(Just learned about it from PR #1882)
Mark t.Helper() where appropriate
|
The 1.12 macOS builder errors out with: Is this known flakiness? (timeout?) Is there a way to kick a build without pushing a new commit? |
|
PS. I think the timeout is a problem with travis, actually. |
cgxxv
pushed a commit
to cgxxv/delve
that referenced
this pull request
Mar 25, 2022
* Remove hardcoded tests * Cleanup - consistent naming, simplify some code, remove unused func * Address review comments * Add Seq checks back * Address more review comments * Remove hardcoded tests * Cleanup - consistent naming, simplify some code, remove unused func * Address review comments * Add Seq checks back * Address more review comments * Make more consistent * More consistency * Simplify Except* methods with a helper Mark t.Helper() where appropriate
abner-chenc
pushed a commit
to loongson/delve
that referenced
this pull request
Mar 1, 2024
* Remove hardcoded tests * Cleanup - consistent naming, simplify some code, remove unused func * Address review comments * Add Seq checks back * Address more review comments * Remove hardcoded tests * Cleanup - consistent naming, simplify some code, remove unused func * Address review comments * Add Seq checks back * Address more review comments * Make more consistent * More consistency * Simplify Except* methods with a helper Mark t.Helper() where appropriate
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DAP tests will check server responses by analyzing interesting fields on a field-by-field basis rather than comparing serialized messages with
[]bytegolden strings.Updates #1515