[GitHub] [beam] youngoli commented on a change in pull request #17058: [BEAM-13888] Add unit testing to ioutilx

[GitBox <gi...@apache.org>]

youngoli commented on a change in pull request #17058:
URL: https://github.com/apache/beam/pull/17058#discussion_r827603953



##########
File path: sdks/go/pkg/beam/core/util/ioutilx/read_test.go
##########
@@ -0,0 +1,94 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements.  See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License.  You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package ioutilx
+
+import (
+	"io"
+	"strings"
+	"testing"
+)
+
+func TestReadN(t *testing.T) {
+	testString := "hello world!"
+	testLength := len(testString)
+	r := strings.NewReader(testString)
+
+	// test good read
+	data, err := ReadN(r, testLength)
+	// err is expected to be nil
+	if err != nil {
+		t.Errorf("failed to read data: got %v", err)

Review comment:
       Nit: When nesting errors, the best convention is to separate errors with colons, like so. Otherwise it's difficult to tell which is your error and which is the nested error. Like in your case, the word "got" would look like it's the first word of the nested error.
   ```suggestion
   		t.Errorf("failed to read data, got error: %v", err)
   ```
   (Note: This is repeated in the other tests too, don't forget to fix all of them.)

##########
File path: sdks/go/pkg/beam/core/util/ioutilx/read_test.go
##########
@@ -0,0 +1,94 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements.  See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License.  You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package ioutilx
+
+import (
+	"io"
+	"strings"
+	"testing"
+)
+
+func TestReadN(t *testing.T) {
+	testString := "hello world!"
+	testLength := len(testString)
+	r := strings.NewReader(testString)
+
+	// test good read
+	data, err := ReadN(r, testLength)
+	// err is expected to be nil
+	if err != nil {
+		t.Errorf("failed to read data: got %v", err)
+	}
+	// length of data is expected to match testLength
+	if got, want := len(data), testLength; got != want {
+		t.Errorf("got length %v, wanted %v", got, want)
+	}
+	// content of data is expected to match testString
+	if got, want := string(data), testString; got != want {
+		t.Errorf("got string %q, wanted %q", got, want)
+	}
+}
+
+func TestReadN_Bad(t *testing.T) {
+	testString := "hello world!"
+	testLength := len(testString)
+	r := strings.NewReader(testString)
+
+	// test bad read
+	_, err := ReadN(r, testLength+1)
+	// err is expected to be io.EOF
+	if err != io.EOF {
+		t.Errorf("got error %v, wanted %v", err, io.EOF)

Review comment:
       Nit: In this case since we have two different errors to output, we'll have to go a little further to make it legible to readers. There aren't any common conventions here that I know of, but I'd suggest adding in a newline separating the errors, like so:
   ```suggestion
   		t.Errorf("got error: %v\nwanted error: %v", err, io.EOF)
   ```
   Although alternatives like quotation marks, brackets, etc. would work in this case too. The important part is that readers can tell where one error ends and another begins.

##########
File path: sdks/go/pkg/beam/core/util/ioutilx/write_test.go
##########
@@ -0,0 +1,38 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements.  See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License.  You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package ioutilx
+
+import (
+	"bufio"
+	"bytes"
+	"testing"
+)
+
+func TestWriteUnsafe(t *testing.T) {
+	var buf bytes.Buffer
+	writer := bufio.NewWriter(&buf)
+	var data = []byte("hello world!")
+
+	length, err := WriteUnsafe(writer, data)
+	// err is expected to be nil
+	if err != nil {
+		t.Errorf("failed to write data: got %v", err)
+	}
+	// length of data is expected to match input
+	if got, want := length, len(data); got != want {
+		t.Errorf("got length %v, wanted %v", got, want)
+	}
+}

Review comment:
       I would expect this test to also check that buf contains the input data. I.e. make sure that WriteUnsafe wrote properly.




-- 
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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org