[GitHub] [beam] youngoli commented on a change in pull request #12554: [BEAM-9615] Map user types to Schema reps.

[GitBox <gi...@apache.org>]

youngoli commented on a change in pull request #12554:
URL: https://github.com/apache/beam/pull/12554#discussion_r470392567



##########
File path: sdks/go/pkg/beam/core/runtime/graphx/schema/schema_test.go
##########
@@ -161,18 +354,22 @@ func TestSchemaConversion(t *testing.T) {
 				if err != nil {
 					t.Fatalf("error ToType(%v) = %v", test.st, err)
 				}
-
-				if d := cmp.Diff(reflect.New(test.rt).Elem().Interface(), reflect.New(got).Elem().Interface()); d != "" {
-					t.Errorf("diff (-want, +got): %v", d)
+				if !test.rt.AssignableTo(got) {
+					t.Errorf("%v not assignable to %v", test.rt, got)
+					if d := cmp.Diff(reflect.New(test.rt).Elem().Interface(), reflect.New(got).Elem().Interface()); d != "" {
+						t.Errorf("diff (-want, +got): %v", d)
+					}
 				}
 			}
 			{
 				got, err := FromType(test.rt)
 				if err != nil {
 					t.Fatalf("error FromType(%v) = %v", test.rt, err)
 				}
-
-				if d := cmp.Diff(test.st, got); d != "" {
+				if d := cmp.Diff(test.st, got,
+					protocmp.Transform(),
+					protocmp.IgnoreFields(proto.MessageV2(&pipepb.Schema{}), "id", "options"),

Review comment:
       I get why this is ignoring the "id" field for the proto, but why "options"? This test seems to already be testing options in the other direction (options are respected when converting from schemas to reflect types). I figure it would also want to test that options are included as expected when converting to schemas.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org