damccorm commented on a change in pull request #17124: URL: https://github.com/apache/beam/pull/17124#discussion_r831091254
########## File path: sdks/go/pkg/beam/core/util/jsonx/jsonx_test.go ########## @@ -0,0 +1,82 @@ +// 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 jsonx + +import ( + "math" + "testing" +) + +type someStruct struct { + SomeNumber int64 + SomeString string + SomeBoolean bool +} + +func (s *someStruct) equals(other *someStruct) bool { + return (s.SomeNumber == other.SomeNumber) && (s.SomeString == other.SomeString) && (s.SomeBoolean == other.SomeBoolean) +} + +func TestEncodeDecode(t *testing.T) { + var tests = []struct { + name string + inputNum int64 + inputString string + inputBool bool + }{ + { + "simple inputs", + 10, + "information", + true, + }, + { + "default inputs", + 0, + "", + false, + }, + { + "maximum int", + math.MaxInt64, + "big int", + true, + }, + { + "minimum int", + math.MinInt64, + "little int", + false, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + inputStruct := &someStruct{SomeNumber: test.inputNum, SomeString: test.inputString, SomeBoolean: test.inputBool} + enc, err := Marshal(inputStruct) + if err != nil { + t.Fatalf("Marshal(%v) failed, got %v", inputStruct, err) + } Review comment: This is a bit nitpicky, but a more robust test would check that this is actually getting encoded to json - right now, all we've tested is that we're able to serialize/deserialize into some protocol, not that its actually valid json. One way to do this would be to compare the byte contents, an easier way is probably to just use the json package. That actually brings me to a larger question - do we actually need this package? Or would we be better off removing references to it (switching to the json package) and deprecating this one. By my count we only have 3 files that use any of this functionality, and all of them could very easily be replaced by calls to the json package. I'm ok taking this pr and not deprecating this, but I think its at least worth considering. ########## File path: sdks/go/pkg/beam/core/util/jsonx/jsonx_test.go ########## @@ -0,0 +1,82 @@ +// 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 jsonx + +import ( + "math" + "testing" +) + +type someStruct struct { + SomeNumber int64 + SomeString string + SomeBoolean bool +} + +func (s *someStruct) equals(other *someStruct) bool { + return (s.SomeNumber == other.SomeNumber) && (s.SomeString == other.SomeString) && (s.SomeBoolean == other.SomeBoolean) +} + +func TestEncodeDecode(t *testing.T) { + var tests = []struct { + name string + inputNum int64 + inputString string + inputBool bool + }{ + { + "simple inputs", + 10, + "information", + true, + }, + { + "default inputs", + 0, + "", + false, + }, + { + "maximum int", + math.MaxInt64, + "big int", + true, + }, + { + "minimum int", + math.MinInt64, + "little int", + false, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + inputStruct := &someStruct{SomeNumber: test.inputNum, SomeString: test.inputString, SomeBoolean: test.inputBool} + enc, err := Marshal(inputStruct) + if err != nil { + t.Fatalf("Marshal(%v) failed, got %v", inputStruct, err) + } Review comment: Also, unrelated to the review - I appreciate the branch name -- 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]
