lostluck commented on code in PR #24811:
URL: https://github.com/apache/beam/pull/24811#discussion_r1059102179
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -0,0 +1,105 @@
+package sql
+
+import (
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/transforms/sql/sqlx"
+ "reflect"
+ "runtime"
+ "testing"
+)
+
+func TestOptions_Add(t *testing.T) {
+ o := options{}
+ str := "this is a string"
+ bytes := []byte{1, 2, 3, 4}
+ testOpt := sqlx.Option{
+ Urn: str,
+ Payload: bytes,
+ }
+ tests := []struct {
+ opt sqlx.Option
+ }{
+ {
+ opt: testOpt,
Review Comment:
Since there's only one case, this test becomes more readable if we avoid the
table test loop.
Alternatively, prefer inlining the definition of testOpt into the tests
slice so case by case details don't leak.
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -0,0 +1,105 @@
+package sql
+
+import (
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/transforms/sql/sqlx"
+ "reflect"
+ "runtime"
+ "testing"
+)
+
+func TestOptions_Add(t *testing.T) {
+ o := options{}
+ str := "this is a string"
+ bytes := []byte{1, 2, 3, 4}
+ testOpt := sqlx.Option{
+ Urn: str,
+ Payload: bytes,
+ }
+ tests := []struct {
+ opt sqlx.Option
+ }{
+ {
+ opt: testOpt,
+ },
+ }
+
+ for _, test := range tests {
+ o.Add(test.opt)
+ if o.customs == nil ||
!reflect.DeepEqual(o.customs[len(o.customs)-1], test.opt) {
+ t.Errorf("Add failed.")
+ }
+ }
+}
+
+func TestInput(t *testing.T) {
+ name1 := "this is a string"
+ collection1 := beam.PCollection{}
+ tests := []struct {
+ name string
+ in beam.PCollection
+ }{
+ {
+ name: name1,
+ in: collection1,
+ },
+ }
+
+ o := &options{inputs: make(map[string]beam.PCollection)}
+ for _, test := range tests {
+ option := Input(test.name, test.in)
+ if option == nil {
+ t.Errorf("%v return a nil value, which is not
expected", runtime.FuncForPC(reflect.ValueOf(TestInput).Pointer()).Name())
Review Comment:
While it's *neat* to us the runtime function magically find the test name,
it's not useful in this context.
As written this will produce a test error string of
`TestInput return a nil value, which is not expected`, which isn't useful,
since one already had to type "TestInput" to get it correct.
Also, it's referring to the test case function itself `TestInput`, not the
function under test `Input`. Simply use the text directly instead of
abstracting it unnecessarily.
If you did intend to get the test name itself, use `t.Name()` instead.
https://pkg.go.dev/testing#T.Name
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -0,0 +1,105 @@
+package sql
+
+import (
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/transforms/sql/sqlx"
+ "reflect"
+ "runtime"
+ "testing"
+)
+
+func TestOptions_Add(t *testing.T) {
+ o := options{}
Review Comment:
Defining this outside of the loop means additional test cases are not going
to be hermetic. They will depend on each other. Test case independence is
preferred. in this case, it means adding it to the top of the loop.
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -0,0 +1,105 @@
+package sql
+
+import (
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/transforms/sql/sqlx"
+ "reflect"
+ "runtime"
+ "testing"
+)
+
+func TestOptions_Add(t *testing.T) {
+ o := options{}
+ str := "this is a string"
+ bytes := []byte{1, 2, 3, 4}
+ testOpt := sqlx.Option{
+ Urn: str,
+ Payload: bytes,
+ }
+ tests := []struct {
+ opt sqlx.Option
+ }{
+ {
+ opt: testOpt,
+ },
+ }
+
+ for _, test := range tests {
+ o.Add(test.opt)
+ if o.customs == nil ||
!reflect.DeepEqual(o.customs[len(o.customs)-1], test.opt) {
+ t.Errorf("Add failed.")
+ }
+ }
+}
+
+func TestInput(t *testing.T) {
+ name1 := "this is a string"
+ collection1 := beam.PCollection{}
+ tests := []struct {
+ name string
+ in beam.PCollection
+ }{
+ {
+ name: name1,
+ in: collection1,
+ },
+ }
+
+ o := &options{inputs: make(map[string]beam.PCollection)}
+ for _, test := range tests {
+ option := Input(test.name, test.in)
+ if option == nil {
+ t.Errorf("%v return a nil value, which is not
expected", runtime.FuncForPC(reflect.ValueOf(TestInput).Pointer()).Name())
+ }
+ option(o)
+ if o.inputs == nil || !reflect.DeepEqual(o.inputs[test.name],
test.in) {
+ t.Errorf("%v did not modify inputs, which is not
expected. Expected: %v. Got: %v",
runtime.FuncForPC(reflect.ValueOf(TestExpansionAddr).Pointer()).Name(),
test.in, o.inputs[test.name])
Review Comment:
Prefer "want" instead of "expected". It means the same thing and is half the
size.
https://google.github.io/styleguide/go/decisions#got-before-want
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -0,0 +1,105 @@
+package sql
+
+import (
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/transforms/sql/sqlx"
+ "reflect"
+ "runtime"
+ "testing"
+)
+
+func TestOptions_Add(t *testing.T) {
+ o := options{}
+ str := "this is a string"
+ bytes := []byte{1, 2, 3, 4}
+ testOpt := sqlx.Option{
+ Urn: str,
+ Payload: bytes,
+ }
+ tests := []struct {
+ opt sqlx.Option
+ }{
+ {
+ opt: testOpt,
+ },
+ }
+
+ for _, test := range tests {
+ o.Add(test.opt)
+ if o.customs == nil ||
!reflect.DeepEqual(o.customs[len(o.customs)-1], test.opt) {
+ t.Errorf("Add failed.")
+ }
+ }
+}
+
+func TestInput(t *testing.T) {
+ name1 := "this is a string"
+ collection1 := beam.PCollection{}
+ tests := []struct {
+ name string
+ in beam.PCollection
+ }{
+ {
+ name: name1,
+ in: collection1,
+ },
+ }
Review Comment:
Same commentary here as the previous test case. Likely don't need to use a
table test here.
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -0,0 +1,105 @@
+package sql
+
+import (
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/transforms/sql/sqlx"
+ "reflect"
+ "runtime"
+ "testing"
+)
+
+func TestOptions_Add(t *testing.T) {
+ o := options{}
+ str := "this is a string"
+ bytes := []byte{1, 2, 3, 4}
+ testOpt := sqlx.Option{
+ Urn: str,
+ Payload: bytes,
+ }
+ tests := []struct {
+ opt sqlx.Option
+ }{
+ {
+ opt: testOpt,
+ },
+ }
+
+ for _, test := range tests {
+ o.Add(test.opt)
+ if o.customs == nil ||
!reflect.DeepEqual(o.customs[len(o.customs)-1], test.opt) {
+ t.Errorf("Add failed.")
+ }
+ }
+}
+
+func TestInput(t *testing.T) {
+ name1 := "this is a string"
+ collection1 := beam.PCollection{}
+ tests := []struct {
+ name string
+ in beam.PCollection
+ }{
+ {
+ name: name1,
+ in: collection1,
+ },
+ }
+
+ o := &options{inputs: make(map[string]beam.PCollection)}
+ for _, test := range tests {
+ option := Input(test.name, test.in)
+ if option == nil {
+ t.Errorf("%v return a nil value, which is not
expected", runtime.FuncForPC(reflect.ValueOf(TestInput).Pointer()).Name())
+ }
+ option(o)
+ if o.inputs == nil || !reflect.DeepEqual(o.inputs[test.name],
test.in) {
+ t.Errorf("%v did not modify inputs, which is not
expected. Expected: %v. Got: %v",
runtime.FuncForPC(reflect.ValueOf(TestExpansionAddr).Pointer()).Name(),
test.in, o.inputs[test.name])
Review Comment:
Note how this approach didn't avoid a copy-paste problem here, this prints
TestExpansionAddr.
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -0,0 +1,105 @@
+package sql
+
+import (
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/transforms/sql/sqlx"
+ "reflect"
+ "runtime"
+ "testing"
+)
+
+func TestOptions_Add(t *testing.T) {
+ o := options{}
+ str := "this is a string"
+ bytes := []byte{1, 2, 3, 4}
+ testOpt := sqlx.Option{
+ Urn: str,
+ Payload: bytes,
+ }
+ tests := []struct {
+ opt sqlx.Option
+ }{
+ {
+ opt: testOpt,
+ },
+ }
+
+ for _, test := range tests {
+ o.Add(test.opt)
+ if o.customs == nil ||
!reflect.DeepEqual(o.customs[len(o.customs)-1], test.opt) {
+ t.Errorf("Add failed.")
Review Comment:
This isn't a great error message. If there were additional test cases, it
wouldn't be possible to know which one is which. A good test error identifies
the input and the function under test, so the failure output avoids a reader
from needing to hunt for the important details.
See https://google.github.io/styleguide/go/decisions#identify-the-input and
https://google.github.io/styleguide/go/decisions#identify-the-function for tips.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
