lostluck commented on code in PR #33711:
URL: https://github.com/apache/beam/pull/33711#discussion_r1926025908
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -94,3 +96,133 @@ func TestExpansionAddr(t *testing.T) {
t.Errorf("The function that ExpansionAddr(%v) returned did not
work correctly. For the expansionAddr field in options, got %v, want %v",
test.addr, o.expansionAddr, test.addr)
}
}
+
+/*
+TestOutputType tests the OutputType option for setting the output type
+in an SQL transformation in Beam. It verifies both cases: when
+components are provided and when they are not. The test checks if
+the 'outType' field in the options is set correctly based on the
+output type and components.
+
+@Author: Mohit Paddhariya
+@Description: This test checks the functionality of OutputType
+option, ensuring that it properly handles setting the output type
+both with and without components.
+
+@Use: This test ensures that the OutputType option behaves as expected
+when used in SQL transformations.
+*/
Review Comment:
In Go, we use // for doc comment blocks, not /* */. In particular,
multi-line comments are generally used sparingly, such as it would make a copy
paste of some command simpler. Please change this for consistency with the rest
of the project.
Please also remove the @ blocked lines, they either repeat the initial test
description, or don't add anything. It's also not the Apache Way to take credit
for any given block of code in the files themselves. That is sufficiently
tracked via this commit existing attached to your Github profile, and the
resulting `blame` lines pointing to it once it is merged in.
The same applies to all blocks below.
See https://google.github.io/styleguide/go/decisions#package-comments
generally for package comments.
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -94,3 +96,133 @@ func TestExpansionAddr(t *testing.T) {
t.Errorf("The function that ExpansionAddr(%v) returned did not
work correctly. For the expansionAddr field in options, got %v, want %v",
test.addr, o.expansionAddr, test.addr)
}
}
+
+/*
+TestOutputType tests the OutputType option for setting the output type
+in an SQL transformation in Beam. It verifies both cases: when
+components are provided and when they are not. The test checks if
+the 'outType' field in the options is set correctly based on the
+output type and components.
+
+@Author: Mohit Paddhariya
+@Description: This test checks the functionality of OutputType
+option, ensuring that it properly handles setting the output type
+both with and without components.
+
+@Use: This test ensures that the OutputType option behaves as expected
+when used in SQL transformations.
+*/
+func TestOutputType(t *testing.T) {
+ o := &options{}
+ typ := reflect.TypeOf(int64(0))
+ components := []typex.FullType{typex.New(reflect.TypeOf(""))}
+
+ // Test without components
+ opt1 := OutputType(typ)
+ opt1(o)
+ expected1 := typex.New(typ)
+ if !reflect.DeepEqual(o.outType, expected1) {
Review Comment:
Prefer to use `cmp.Equal` or `cmp.Diff` instead of `reflect.DeepEqual`.
Consider printing the diff, which will be a clearer description of the issue.
See https://google.github.io/styleguide/go/decisions#print-diffs
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -94,3 +96,133 @@ func TestExpansionAddr(t *testing.T) {
t.Errorf("The function that ExpansionAddr(%v) returned did not
work correctly. For the expansionAddr field in options, got %v, want %v",
test.addr, o.expansionAddr, test.addr)
}
}
+
+/*
+TestOutputType tests the OutputType option for setting the output type
+in an SQL transformation in Beam. It verifies both cases: when
+components are provided and when they are not. The test checks if
+the 'outType' field in the options is set correctly based on the
+output type and components.
+
+@Author: Mohit Paddhariya
+@Description: This test checks the functionality of OutputType
+option, ensuring that it properly handles setting the output type
+both with and without components.
+
+@Use: This test ensures that the OutputType option behaves as expected
+when used in SQL transformations.
+*/
+func TestOutputType(t *testing.T) {
+ o := &options{}
+ typ := reflect.TypeOf(int64(0))
+ components := []typex.FullType{typex.New(reflect.TypeOf(""))}
+
+ // Test without components
Review Comment:
In go, prefer using Table Tests instead of single test bodies with comments
for each test case.
See https://google.github.io/styleguide/go/decisions#table-driven-tests
specifically, and
https://google.github.io/styleguide/go/decisions#test-structure more generally.
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -94,3 +96,133 @@ func TestExpansionAddr(t *testing.T) {
t.Errorf("The function that ExpansionAddr(%v) returned did not
work correctly. For the expansionAddr field in options, got %v, want %v",
test.addr, o.expansionAddr, test.addr)
}
}
+
+/*
+TestOutputType tests the OutputType option for setting the output type
+in an SQL transformation in Beam. It verifies both cases: when
+components are provided and when they are not. The test checks if
+the 'outType' field in the options is set correctly based on the
+output type and components.
+
+@Author: Mohit Paddhariya
+@Description: This test checks the functionality of OutputType
+option, ensuring that it properly handles setting the output type
+both with and without components.
+
+@Use: This test ensures that the OutputType option behaves as expected
+when used in SQL transformations.
+*/
+func TestOutputType(t *testing.T) {
+ o := &options{}
+ typ := reflect.TypeOf(int64(0))
+ components := []typex.FullType{typex.New(reflect.TypeOf(""))}
+
+ // Test without components
+ opt1 := OutputType(typ)
+ opt1(o)
+ expected1 := typex.New(typ)
+ if !reflect.DeepEqual(o.outType, expected1) {
+ t.Errorf("OutputType() without components failed: got %v, want
%v", o.outType, expected1)
+ }
+
+ // Test with components
+ opt2 := OutputType(typ, components...)
+ opt2(o)
+ expected2 := typex.New(typ, components...)
+ if !reflect.DeepEqual(o.outType, expected2) {
+ t.Errorf("OutputType() with components failed: got %v, want
%v", o.outType, expected2)
+ }
+}
+
+/*
+TestTransform tests the behavior of the Transform function
+in the context of SQL transformations. Specifically, it checks
+that the function panics when an output type is missing.
+
+@Author: Mohit Paddhariya
+@Description: This test verifies that Transform() panics when the
+output type is not specified for a SQL transformation.
+
+@Use: Ensures that missing output type results in a panic,
+helping to catch incorrect or incomplete transformations.
+*/
+func TestTransform(t *testing.T) {
+ // Test just the panic case for missing output type
+ // as we can't easily test the actual transformation without an
expansion service
+ t.Run("Missing output type", func(t *testing.T) {
+ p := beam.NewPipeline()
+ s := p.Root()
+ col := beam.Create(s, 1, 2, 3)
+
+ defer func() {
+ r := recover()
+ if r == nil {
+ t.Error("Transform() with missing output type
should panic")
+ }
+ if msg, ok := r.(string); !ok || msg != "output type
must be specified for sql.Transform" {
+ t.Errorf("Transform() unexpected panic message:
%v", r)
+ }
+ }()
+
+ Transform(s, "SELECT value FROM test", Input("test", col))
+ })
+}
+
+/*
+TestMultipleOptions tests applying multiple options at once
+and verifying that they are all correctly applied to the options object.
+
+@Author: Mohit Paddhariya
+@Description: This test ensures that multiple options, including
+Input, Dialect, ExpansionAddr, and OutputType, are correctly
+applied to the options object. It also verifies that a custom
+option can be added and applied.
+
+@Use: Ensures that multiple options can be applied in sequence and
+all the fields in the options object are correctly set.
+*/
+func TestMultipleOptions(t *testing.T) {
+ o := &options{
+ inputs: make(map[string]beam.PCollection),
+ }
+
+ p := beam.NewPipeline()
+ s := p.Root()
+ col := beam.Create(s, 1, 2, 3)
+ name := "test"
+ dialect := "zetasql"
+ addr := "localhost:8080"
+ typ := reflect.TypeOf(int64(0))
+ customOpt := sqlx.Option{Urn: "test"}
+
+ // Apply multiple options
+ opts := []Option{
+ Input(name, col),
+ Dialect(dialect),
+ ExpansionAddr(addr),
+ OutputType(typ),
+ }
+
+ // Apply all options
+ for _, opt := range opts {
+ opt(o)
+ }
+ o.Add(customOpt)
+
+ // Verify all fields
+ if _, ok := o.inputs[name]; !ok {
+ t.Error("Input option not applied correctly")
Review Comment:
Test output is much better when the input, both the received and the
expected outputs are included in the test message.
"Input option with name %q, not applied correctly: got %v, want %v", name,
got, want"
And similar.
See also https://mtlynch.io/if-got-want-improve-go-tests/ which is an
excellent approach for this sort of thing too.
--
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]
