fishy commented on a change in pull request #2293:
URL: https://github.com/apache/thrift/pull/2293#discussion_r542916225
##########
File path: lib/go/thrift/http_client_test.go
##########
@@ -103,6 +105,54 @@ func TestHttpCustomClientPackageScope(t *testing.T) {
}
}
+func TestHTTPClientFlushesRequestBufferOnErrors(t *testing.T) {
+ var (
+ write1 = []byte("write 1")
+ write2 = []byte("write 2")
+ )
+
+ l, addr := HttpClientSetupForTest(t)
+ if l != nil {
+ defer l.Close()
+ }
+ trans, err := NewTHttpPostClient("http://"; + addr.String())
+ if err != nil {
+ t.Fatalf("Unable to connect to %s: %s", addr.String(), err)
Review comment:
This is nitpicking and also a problem already exists in the old test
code, but using `%s` on errors is the same as using `%v` on errors except that
`%s` handles it much more poorer when error is nil
(https://play.golang.org/p/yHovIp3m37B). I'd rather just use `%v` with errors
instead.
##########
File path: lib/go/thrift/http_client_test.go
##########
@@ -103,6 +105,54 @@ func TestHttpCustomClientPackageScope(t *testing.T) {
}
}
+func TestHTTPClientFlushesRequestBufferOnErrors(t *testing.T) {
+ var (
+ write1 = []byte("write 1")
+ write2 = []byte("write 2")
+ )
+
+ l, addr := HttpClientSetupForTest(t)
+ if l != nil {
+ defer l.Close()
+ }
+ trans, err := NewTHttpPostClient("http://"; + addr.String())
+ if err != nil {
+ t.Fatalf("Unable to connect to %s: %s", addr.String(), err)
+ }
+
+ _, err = trans.Write(write1)
+ if err != nil {
+ t.Fatalf("Failed to write to transport: %s", err.Error())
Review comment:
Similar to the comment above, instead of using `%s` with `err.Error()`,
just use `%v` with `err`.
##########
File path: lib/go/thrift/http_client_test.go
##########
@@ -103,6 +105,54 @@ func TestHttpCustomClientPackageScope(t *testing.T) {
}
}
+func TestHTTPClientFlushesRequestBufferOnErrors(t *testing.T) {
+ var (
+ write1 = []byte("write 1")
+ write2 = []byte("write 2")
+ )
+
+ l, addr := HttpClientSetupForTest(t)
+ if l != nil {
+ defer l.Close()
+ }
+ trans, err := NewTHttpPostClient("http://"; + addr.String())
+ if err != nil {
+ t.Fatalf("Unable to connect to %s: %s", addr.String(), err)
+ }
+
+ _, err = trans.Write(write1)
+ if err != nil {
+ t.Fatalf("Failed to write to transport: %s", err.Error())
+ }
+ ctx, cancel := context.WithCancel(context.Background())
+ cancel()
+ err = trans.Flush(ctx)
+ if err == nil {
+ t.Fatalf("Expected flush error")
+ }
+
+ _, err = trans.Write(write2)
+ if err != nil {
+ t.Fatalf("Failed to write to transport: %s", err.Error())
+ }
+ err = trans.Flush(context.Background())
+ if err != nil {
+ t.Fatalf("Failed to flush: %s", err.Error())
+ }
+
+ data := make([]byte, 1024)
+ n, err := trans.Read(data)
+ if err != nil {
+ t.Fatalf("Failed to read: %s", err.Error())
+ }
+
+ data = data[:n]
+ if !bytes.Equal(data, write2) {
+ t.Fatalf("Received unexpected data: %s", data)
Review comment:
My suggestions here are:
1. Use `%q` instead of `%s` (sometimes when the test failed it's because you
read zero data, with `%s` the error message will show as `Received unexpected
data: `, while with `%q` it shows as `Received unexpected data: ""` instead,
which makes it much more obvious that the failure is due to empty string).
2. Also print out what's the expected data in the failure message (e.g.
`t.Fatalf("Received data expected %q, got %q", write2, data)`)
##########
File path: lib/go/thrift/http_client_test.go
##########
@@ -103,6 +105,54 @@ func TestHttpCustomClientPackageScope(t *testing.T) {
}
}
+func TestHTTPClientFlushesRequestBufferOnErrors(t *testing.T) {
+ var (
+ write1 = []byte("write 1")
+ write2 = []byte("write 2")
+ )
+
+ l, addr := HttpClientSetupForTest(t)
+ if l != nil {
+ defer l.Close()
+ }
+ trans, err := NewTHttpPostClient("http://"; + addr.String())
+ if err != nil {
+ t.Fatalf("Unable to connect to %s: %s", addr.String(), err)
+ }
+
+ _, err = trans.Write(write1)
+ if err != nil {
+ t.Fatalf("Failed to write to transport: %s", err.Error())
+ }
+ ctx, cancel := context.WithCancel(context.Background())
+ cancel()
+ err = trans.Flush(ctx)
+ if err == nil {
+ t.Fatalf("Expected flush error")
+ }
+
+ _, err = trans.Write(write2)
+ if err != nil {
+ t.Fatalf("Failed to write to transport: %s", err.Error())
+ }
+ err = trans.Flush(context.Background())
+ if err != nil {
+ t.Fatalf("Failed to flush: %s", err.Error())
+ }
+
+ data := make([]byte, 1024)
+ n, err := trans.Read(data)
+ if err != nil {
+ t.Fatalf("Failed to read: %s", err.Error())
+ }
+
+ data = data[:n]
+ if !bytes.Equal(data, write2) {
+ t.Fatalf("Received unexpected data: %s", data)
+ }
+ trans.Close()
Review comment:
Please move this to right after you checked for error on Line 121 as
`defer trans.Close()`. You have a lot of `t.Fatalf` that could exit the test
early without closing the opened trans (this is test code so this is
nitpicking).
##########
File path: lib/go/thrift/http_client_test.go
##########
@@ -103,6 +105,54 @@ func TestHttpCustomClientPackageScope(t *testing.T) {
}
}
+func TestHTTPClientFlushesRequestBufferOnErrors(t *testing.T) {
+ var (
+ write1 = []byte("write 1")
+ write2 = []byte("write 2")
+ )
+
+ l, addr := HttpClientSetupForTest(t)
+ if l != nil {
+ defer l.Close()
+ }
+ trans, err := NewTHttpPostClient("http://"; + addr.String())
+ if err != nil {
+ t.Fatalf("Unable to connect to %s: %s", addr.String(), err)
+ }
+
+ _, err = trans.Write(write1)
+ if err != nil {
+ t.Fatalf("Failed to write to transport: %s", err.Error())
+ }
+ ctx, cancel := context.WithCancel(context.Background())
+ cancel()
+ err = trans.Flush(ctx)
+ if err == nil {
+ t.Fatalf("Expected flush error")
Review comment:
There's no formatting verbs so this can be `t.Fatal` instead of
`t.Fatalf`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
