[BEAM-13888] Add unit testing to ioutilx #17058

Abacn · 2022-03-09T21:15:07Z

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

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

* Add basic unit testing to the go ioutilx package

jrmccluskey

Good start! Have some style changes for you to make and a few things that leverage Go features.

jrmccluskey · 2022-03-09T21:58:33Z

sdks/go/pkg/beam/core/util/ioutilx/read_test.go

+	data, err := ReadN(r, testLength)
+	// err is expected to be nil
+	if err != nil {
+		t.Errorf("Failed to read data: %v", err)


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

Suggested change

t.Errorf("Failed to read data: %v", err)

t.Errorf("failed to read data: got %v", err)

jrmccluskey · 2022-03-09T21:59:04Z

sdks/go/pkg/beam/core/util/ioutilx/read_test.go

+)
+
+func TestReadN(t *testing.T) {
+	const testString = "hello world!"


No need to use consts here

Suggested change

const testString = "hello world!"

testString := "hello world!"

jrmccluskey · 2022-03-09T21:59:45Z

sdks/go/pkg/beam/core/util/ioutilx/read_test.go

+		t.Errorf("Got %q, wanted %q", string(data), testString)
+	}
+
+	// test bad read


Split the bad read case into a separate test case called TestReadN_bad

jrmccluskey · 2022-03-09T22:02:08Z

sdks/go/pkg/beam/core/util/ioutilx/read_test.go

+		t.Errorf("Failed to read data: %v", err)
+	}
+	// length of data is expected to match testLength
+	if (len(data) != testLength) {


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:

Suggested change

if (len(data) != testLength) {

if got, want := len(data), testLength; got != want {

t.Errorf("got length %v, want %v", got, want)

}

jrmccluskey · 2022-03-09T22:02:40Z

sdks/go/pkg/beam/core/util/ioutilx/read_test.go

+		t.Errorf("Got %v, wanted %v", len(data), testLength)
+	}
+	// content of data is expected to match testString
+	if (string(data) != testString) {


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

jrmccluskey · 2022-03-09T22:03:32Z

sdks/go/pkg/beam/core/util/ioutilx/read_test.go

+	}
+}
+
+func TestReadUnsafe(t *testing.T) {


Same style comments as above

jrmccluskey · 2022-03-09T22:03:43Z

sdks/go/pkg/beam/core/util/ioutilx/read_test.go

+	}
+}
+
+func TestReadNBufUnsafe(t *testing.T) {


Same style comments as above

jrmccluskey · 2022-03-09T22:06:43Z

sdks/go/pkg/beam/core/util/ioutilx/read_test.go

+	r := strings.NewReader(testString)
+	var data [testLength]byte
+
+	length, err := ReadUnsafe(r, data[:])


You should be able to pass data here without specifying the range for the slice.

Suggested change

length, err := ReadUnsafe(r, data[:])

length, err := ReadUnsafe(r, data)

jrmccluskey · 2022-03-09T22:07:24Z

sdks/go/pkg/beam/core/util/ioutilx/write_test.go

+)
+
+func TestWriteUnsafe(t *testing.T) {
+	var buff bytes.Buffer


Nit: Go style usually calls this buf

jrmccluskey · 2022-03-09T22:07:57Z

sdks/go/pkg/beam/core/util/ioutilx/write_test.go

+	"testing"
+)
+
+func TestWriteUnsafe(t *testing.T) {


Same style comments as in write.

jrmccluskey · 2022-03-09T22:09:45Z

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.

github-actions · 2022-03-09T22:35:20Z

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)

Abacn · 2022-03-10T16:49:42Z

@jrmccluskey Thanks for feedbacks! Comments addressed.

sdks/go/pkg/beam/core/util/ioutilx/read_test.go

jrmccluskey

LGTM, going to add a committer so the workflows can run and we can get this merged in!

Abacn · 2022-03-10T16:57:40Z

Sorry, missed a few style change request and fixed.

jrmccluskey · 2022-03-10T16:58:33Z

R: @youngoli

github-actions · 2022-03-10T16:59:36Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

johnjcasey · 2022-03-14T20:46:47Z

LGTM

youngoli

Looks really good. I'm being a bit more nitpicky than I usually would be just to explain Go conventions, but even then this is nearly there.

youngoli · 2022-03-16T03:47:32Z

sdks/go/pkg/beam/core/util/ioutilx/write_test.go

+	if got, want := length, len(data); got != want {
+		t.Errorf("got length %v, wanted %v", got, want)
+	}
+}


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

youngoli · 2022-03-16T03:53:55Z

sdks/go/pkg/beam/core/util/ioutilx/read_test.go

+	data, err := ReadN(r, testLength)
+	// err is expected to be nil
+	if err != nil {
+		t.Errorf("failed to read data: got %v", err)


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.

Suggested change

t.Errorf("failed to read data: got %v", err)

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

youngoli · 2022-03-16T03:58:35Z

sdks/go/pkg/beam/core/util/ioutilx/read_test.go

+	_, 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)


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:

Suggested change

t.Errorf("got error %v, wanted %v", err, io.EOF)

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.

* added writer test for content * message readibility fix

Abacn · 2022-03-16T15:06:16Z

@youngoli Thanks for feedbacks! Comments addressed.

youngoli

Looks good to me!

[BREAM-13888] Add unit testing to ioutilx

57dd2a5

* Add basic unit testing to the go ioutilx package

github-actions bot added the go label Mar 9, 2022

jrmccluskey requested changes Mar 9, 2022

View reviewed changes

Abacn changed the title ~~[BREAM-13888] Add unit testing to ioutilx~~ [BEAM-13888] Add unit testing to ioutilx Mar 9, 2022

github-actions bot added the Next Action: Reviewers label Mar 9, 2022

style fix

1356a60

jrmccluskey reviewed Mar 10, 2022

View reviewed changes

sdks/go/pkg/beam/core/util/ioutilx/read_test.go Outdated Show resolved Hide resolved

Missed style fix

a642409

jrmccluskey approved these changes Mar 10, 2022

View reviewed changes

Missed style fix (redundant slice range operator)

5db1526

youngoli requested changes Mar 16, 2022

View reviewed changes

Address comments

f414ee4

* added writer test for content * message readibility fix

youngoli approved these changes Mar 16, 2022

View reviewed changes

youngoli merged commit 15e636f into apache:master Mar 16, 2022

Abacn deleted the yhu-BEAM-13888-ioutilxUnitTest branch March 16, 2022 18:29

	t.Errorf("Failed to read data: %v", err)
	t.Errorf("failed to read data: got %v", err)

	const testString = "hello world!"
	testString := "hello world!"

-	if (len(data) != testLength) {
+	if got, want := len(data),  testLength; got != want {
+	     t.Errorf("got length %v, want %v", got, want)
+	}

	length, err := ReadUnsafe(r, data[:])
	length, err := ReadUnsafe(r, data)

	t.Errorf("failed to read data: got %v", err)
	t.Errorf("failed to read data, got error: %v", err)

	t.Errorf("got error %v, wanted %v", err, io.EOF)
	t.Errorf("got error: %v\nwanted error: %v", err, io.EOF)

[BEAM-13888] Add unit testing to ioutilx #17058

[BEAM-13888] Add unit testing to ioutilx #17058

Uh oh!

Conversation

Abacn commented Mar 9, 2022

GitHub Actions Tests Status (on master branch)

Uh oh!

jrmccluskey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrmccluskey commented Mar 9, 2022

Uh oh!

github-actions bot commented Mar 9, 2022

Uh oh!

Abacn commented Mar 10, 2022

Uh oh!

Uh oh!

jrmccluskey left a comment

Choose a reason for hiding this comment

Uh oh!

Abacn commented Mar 10, 2022

Uh oh!

jrmccluskey commented Mar 10, 2022

Uh oh!

github-actions bot commented Mar 10, 2022

Uh oh!

johnjcasey commented Mar 14, 2022

Uh oh!

youngoli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abacn commented Mar 16, 2022

Uh oh!

youngoli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants