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 mock server outline and test #19

Merged
merged 4 commits into from
Dec 19, 2019
Merged

Add mock server outline and test #19

merged 4 commits into from
Dec 19, 2019

Conversation

polinasok
Copy link
Contributor

No description provided.

cmd/mockserver/io.go Outdated Show resolved Hide resolved
cmd/mockserver/io.go Outdated Show resolved Hide resolved
cmd/mockserver/main.go Outdated Show resolved Hide resolved
cmd/mockserver/main.go Outdated Show resolved Hide resolved
cmd/mockserver/main_test.go Outdated Show resolved Hide resolved
cmd/mockserver/server.go Outdated Show resolved Hide resolved
cmd/mockserver/server.go Outdated Show resolved Hide resolved
cmd/mockserver/server.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@polinasok polinasok left a comment

Choose a reason for hiding this comment

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

PTAL

Copy link
Member

@eliben eliben left a comment

Choose a reason for hiding this comment

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

Small comments with one more major suggestion for TODO/future refactoring

cmd/mockserver/io.go Outdated Show resolved Hide resolved
cmd/mockserver/server.go Outdated Show resolved Hide resolved
cmd/mockserver/server.go Outdated Show resolved Hide resolved
}
writeProtocolMessage(rw.Writer, response)
log.Printf("Response sent\n\t%#v\n", response)
rw.Flush()
Copy link
Member

Choose a reason for hiding this comment

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

I was looking for this line :)

In general, I don't see a reason to buffer the writer here. The code seems to be using bufio.ReadWriter only to bunch together the reader and the writer, but the writer doesn't have to be buffered.

The code can go in as is for now, but please add a TODO/issue to refactor this. I would suggest making server a type with methods, and the writer and bufio.Reader to be struct fields in the type. The methods can then simply access them.

Then you also wouldn't have to pass these readers/writers around to handlers, dispatchers, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #21

Copy link
Contributor Author

@polinasok polinasok left a comment

Choose a reason for hiding this comment

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

PTAL

@polinasok polinasok assigned eliben and unassigned polinasok Dec 19, 2019
@@ -105,3 +106,21 @@ func readContentLengthHeader(r *bufio.Reader) (contentLength int, err error) {
}
return strconv.Atoi(headerAndLength[1])
}

// WriteProtocolMessage encodes message and writes it to w.
func WriteProtocolMessage(w io.Writer, message Message) error {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: now that these higher level functions are in io.go, add an issue to consider making the other functions unexported. It's unlikely that we'll want clients to use them directly, and for tests it doesn't matter (because they're in the same package). It's always possible to export a new function w/o breaking the API, whereas the reverse is not possible. So "hidden by default" is safer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #22

@eliben eliben assigned polinasok and unassigned eliben Dec 19, 2019
@polinasok polinasok merged commit d33e5bb into google:master Dec 19, 2019
@polinasok polinasok deleted the mock branch January 9, 2020 20:50
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.

2 participants