You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/03/09 21:15:14 UTC
[GitHub] [beam] Abacn opened a new pull request #17058: [BREAM-13888] Add unit testing to ioutilx
Abacn opened a new pull request #17058:
URL: https://github.com/apache/beam/pull/17058
Add unit testing for everything in ioutilx package
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment ( @jrmccluskey ).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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
[GitHub] [beam] jrmccluskey commented on pull request #17058: [BREAM-13888] Add unit testing to ioutilx
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on pull request #17058:
URL: https://github.com/apache/beam/pull/17058#issuecomment-1063422978
Also: fix the typo in the PR title so it gets mapped to the Jira ticket properly, and when you request a reviewer you do it in a follow-up comment and not the body of the PR template.
--
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
[GitHub] [beam] johnjcasey commented on pull request #17058: [BEAM-13888] Add unit testing to ioutilx
Posted by GitBox <gi...@apache.org>.
johnjcasey commented on pull request #17058:
URL: https://github.com/apache/beam/pull/17058#issuecomment-1067270949
LGTM
--
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
[GitHub] [beam] youngoli merged pull request #17058: [BEAM-13888] Add unit testing to ioutilx
Posted by GitBox <gi...@apache.org>.
youngoli merged pull request #17058:
URL: https://github.com/apache/beam/pull/17058
--
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
[GitHub] [beam] github-actions[bot] commented on pull request #17058: [BEAM-13888] Add unit testing to ioutilx
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17058:
URL: https://github.com/apache/beam/pull/17058#issuecomment-1063444227
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @jrmccluskey for label go.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
--
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
[GitHub] [beam] youngoli commented on a change in pull request #17058: [BEAM-13888] Add unit testing to ioutilx
Posted by 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
[GitHub] [beam] Abacn commented on pull request #17058: [BEAM-13888] Add unit testing to ioutilx
Posted by GitBox <gi...@apache.org>.
Abacn commented on pull request #17058:
URL: https://github.com/apache/beam/pull/17058#issuecomment-1069229183
@youngoli Thanks for feedbacks! Comments addressed.
--
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
[GitHub] [beam] github-actions[bot] commented on pull request #17058: [BEAM-13888] Add unit testing to ioutilx
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17058:
URL: https://github.com/apache/beam/pull/17058#issuecomment-1064288093
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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
[GitHub] [beam] Abacn commented on pull request #17058: [BEAM-13888] Add unit testing to ioutilx
Posted by GitBox <gi...@apache.org>.
Abacn commented on pull request #17058:
URL: https://github.com/apache/beam/pull/17058#issuecomment-1064286236
Sorry, missed a few style change request and fixed.
--
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
[GitHub] [beam] jrmccluskey commented on pull request #17058: [BEAM-13888] Add unit testing to ioutilx
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on pull request #17058:
URL: https://github.com/apache/beam/pull/17058#issuecomment-1064287068
R: @youngoli
--
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
[GitHub] [beam] jrmccluskey commented on a change in pull request #17058: [BEAM-13888] Add unit testing to ioutilx
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on a change in pull request #17058:
URL: https://github.com/apache/beam/pull/17058#discussion_r823939201
##########
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)
+ }
+}
+
+func TestReadNBufUnsafe(t *testing.T) {
+ testString := "hello world!"
+ testLength := len(testString)
+ r := strings.NewReader(testString)
+ data := make([]byte, testLength)
+
+ err := ReadNBufUnsafe(r, data)
+ // err is expected to be nil
+ if err != nil {
+ t.Errorf("failed to read data: got %v", err)
+ }
+ // content of data is expected to match testString
+ if got, want := string(data[:]), testString; got != want {
Review comment:
```suggestion
if got, want := string(data), testString; got != want {
```
--
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
[GitHub] [beam] Abacn commented on pull request #17058: [BEAM-13888] Add unit testing to ioutilx
Posted by GitBox <gi...@apache.org>.
Abacn commented on pull request #17058:
URL: https://github.com/apache/beam/pull/17058#issuecomment-1064278264
@jrmccluskey Thanks for feedbacks! Comments addressed.
--
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
[GitHub] [beam] jrmccluskey commented on a change in pull request #17058: [BREAM-13888] Add unit testing to ioutilx
Posted by 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