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

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

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



##########
File path: sdks/go/pkg/beam/core/util/ioutilx/read_test.go
##########
@@ -0,0 +1,88 @@
+// 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) {
+	const testString = "hello world!"

Review comment:
       No need to use consts here
   ```suggestion
   	testString := "hello world!"
   ```

##########
File path: sdks/go/pkg/beam/core/util/ioutilx/read_test.go
##########
@@ -0,0 +1,88 @@
+// 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) {
+	const testString = "hello world!"
+	const 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: %v", err)

Review comment:
       Go style is to have lowercase first words thanks to how errors can nest. You' also want to use "got" and "want" a lot - see https://github.com/golang/go/wiki/TestComments#got-before-want
   
   ```suggestion
   		t.Errorf("failed to read data: got %v", err)
   ```

##########
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) {

Review comment:
       Same style comments as in write.

##########
File path: sdks/go/pkg/beam/core/util/ioutilx/read_test.go
##########
@@ -0,0 +1,88 @@
+// 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) {
+	const testString = "hello world!"
+	const 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: %v", err)
+	}
+	// length of data is expected to match testLength
+	if (len(data) != testLength) {
+		t.Errorf("Got %v, wanted %v", len(data), testLength)
+	}
+	// content of data is expected to match testString
+	if (string(data) != testString) {
+		t.Errorf("Got %q, wanted %q", string(data), testString)
+	}
+
+	// test bad read
+	data, err = ReadN(r, testLength+1)
+	// err is expected to be io.EOF
+	if err != io.EOF {
+		t.Errorf("Got %v, wanted %v", err, io.EOF)
+	}
+}
+
+func TestReadNBufUnsafe(t *testing.T) {

Review comment:
       Same style comments as above

##########
File path: sdks/go/pkg/beam/core/util/ioutilx/read_test.go
##########
@@ -0,0 +1,88 @@
+// 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) {
+	const testString = "hello world!"
+	const 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: %v", err)
+	}
+	// length of data is expected to match testLength
+	if (len(data) != testLength) {
+		t.Errorf("Got %v, wanted %v", len(data), testLength)
+	}
+	// content of data is expected to match testString
+	if (string(data) != testString) {
+		t.Errorf("Got %q, wanted %q", string(data), testString)
+	}
+
+	// test bad read
+	data, err = ReadN(r, testLength+1)
+	// err is expected to be io.EOF
+	if err != io.EOF {
+		t.Errorf("Got %v, wanted %v", err, io.EOF)
+	}
+}
+
+func TestReadNBufUnsafe(t *testing.T) {
+	const testString = "hello world!"
+	const testLength = len(testString)
+	r := strings.NewReader(testString)
+	var data [testLength]byte
+
+	err := ReadNBufUnsafe(r, data[:])
+	// err is expected to be nil
+	if err != nil {
+		t.Errorf("Failed to read data: %v", err)
+	}
+	// content of data is expected to match testString
+	if (string(data[:]) != testString) {
+		t.Errorf("Got %q, wanted %q", string(data[:]), testString)
+	}
+}
+
+func TestReadUnsafe(t *testing.T) {
+	const testString = "hello world!"
+	const testLength = len(testString)
+	r := strings.NewReader(testString)
+	var data [testLength]byte
+
+	length, err := ReadUnsafe(r, data[:])

Review comment:
       You should be able to pass `data` here without specifying the range for the slice. 
   
   ```suggestion
   	length, err := ReadUnsafe(r, data)
   ```

##########
File path: sdks/go/pkg/beam/core/util/ioutilx/read_test.go
##########
@@ -0,0 +1,88 @@
+// 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) {
+	const testString = "hello world!"
+	const 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: %v", err)
+	}
+	// length of data is expected to match testLength
+	if (len(data) != testLength) {
+		t.Errorf("Got %v, wanted %v", len(data), testLength)
+	}
+	// content of data is expected to match testString
+	if (string(data) != testString) {
+		t.Errorf("Got %q, wanted %q", string(data), testString)
+	}
+
+	// test bad read
+	data, err = ReadN(r, testLength+1)
+	// err is expected to be io.EOF
+	if err != io.EOF {
+		t.Errorf("Got %v, wanted %v", err, io.EOF)
+	}
+}
+
+func TestReadNBufUnsafe(t *testing.T) {
+	const testString = "hello world!"
+	const testLength = len(testString)
+	r := strings.NewReader(testString)
+	var data [testLength]byte
+
+	err := ReadNBufUnsafe(r, data[:])
+	// err is expected to be nil
+	if err != nil {
+		t.Errorf("Failed to read data: %v", err)
+	}
+	// content of data is expected to match testString
+	if (string(data[:]) != testString) {
+		t.Errorf("Got %q, wanted %q", string(data[:]), testString)
+	}
+}
+
+func TestReadUnsafe(t *testing.T) {

Review comment:
       Same style comments as above

##########
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 buff bytes.Buffer

Review comment:
       Nit: Go style usually calls this `buf` 

##########
File path: sdks/go/pkg/beam/core/util/ioutilx/read_test.go
##########
@@ -0,0 +1,88 @@
+// 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) {
+	const testString = "hello world!"
+	const 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: %v", err)
+	}
+	// length of data is expected to match testLength
+	if (len(data) != testLength) {

Review comment:
       No need to use the parentheses here, and you can use local variable assignment within the if statement to make your error message formatting more concise:
   ```suggestion
   	if got, want := len(data),  testLength; got != want {
   	     t.Errorf("got length %v, want %v", got, want)
   	}
   ```

##########
File path: sdks/go/pkg/beam/core/util/ioutilx/read_test.go
##########
@@ -0,0 +1,88 @@
+// 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) {
+	const testString = "hello world!"
+	const 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: %v", err)
+	}
+	// length of data is expected to match testLength
+	if (len(data) != testLength) {
+		t.Errorf("Got %v, wanted %v", len(data), testLength)
+	}
+	// content of data is expected to match testString
+	if (string(data) != testString) {
+		t.Errorf("Got %q, wanted %q", string(data), testString)
+	}
+
+	// test bad read

Review comment:
       Split the bad read case into a separate test case called TestReadN_bad

##########
File path: sdks/go/pkg/beam/core/util/ioutilx/read_test.go
##########
@@ -0,0 +1,88 @@
+// 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) {
+	const testString = "hello world!"
+	const 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: %v", err)
+	}
+	// length of data is expected to match testLength
+	if (len(data) != testLength) {
+		t.Errorf("Got %v, wanted %v", len(data), testLength)
+	}
+	// content of data is expected to match testString
+	if (string(data) != testString) {

Review comment:
       Same pattern with the parentheses and "got, want" style here




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