Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

features/unmarshal_unique: fix codegen for keys/values #148

Merged
merged 4 commits into from
Nov 21, 2024

Conversation

coxley
Copy link
Contributor

@coxley coxley commented Nov 20, 2024

Summary

Fixes #147

When deciding to intern map keys or values, we were checking if the key or value itself had the extension set. Make it so that we check the map field + tests to confirm behavior.

Additionally, our tests here weren't even verifying the behavior we want! require.Equal will dig past the pointers to compare the underlying bytes. We need require.Same. Before this, the test cases pass even if unique is disabled.

Refs:

Also included unique.proto to make gen-testproto.

Test Plan

>  make test
go install -tags protolegacy ./cmd/protoc-gen-go-vtproto
go install -tags protolegacy google.golang.org/protobuf/cmd/protoc-gen-go
/Users/coxley/projects/vtprotobuf-fork/_vendor/protobuf-21.12/src/protoc \
                --proto_path=/Users/coxley/projects/vtprotobuf-fork/_vendor/protobuf-21.12 \
                --go_out=conformance --plugin protoc-gen-go="/Users/coxley/projects/vtprotobuf-fork/bin/protoc-gen-go" \
                --go-vtproto_out=conformance --plugin protoc-gen-go-vtproto="/Users/coxley/projects/vtprotobuf-fork/bin/protoc-gen-go-vtproto" \
                -I/Users/coxley/projects/vtprotobuf-fork/_vendor/protobuf-21.12/src \
                --go_opt=Msrc/google/protobuf/test_messages_proto2.proto=internal/conformance \
                --go_opt=Msrc/google/protobuf/test_messages_proto3.proto=internal/conformance \
                --go_opt=Mconformance/conformance.proto=internal/conformance \
                --go-vtproto_opt=Msrc/google/protobuf/test_messages_proto2.proto=internal/conformance \
                --go-vtproto_opt=Msrc/google/protobuf/test_messages_proto3.proto=internal/conformance \
                --go-vtproto_opt=Mconformance/conformance.proto=internal/conformance \
                src/google/protobuf/test_messages_proto2.proto \
                src/google/protobuf/test_messages_proto3.proto \
                conformance/conformance.proto
go test -short ./...
?       github.com/planetscale/vtprotobuf/cmd/protoc-gen-go-vtproto     [no test files]
?       github.com/planetscale/vtprotobuf/codec/drpc    [no test files]
?       github.com/planetscale/vtprotobuf/codec/grpc    [no test files]
?       github.com/planetscale/vtprotobuf/features/clone        [no test files]
?       github.com/planetscale/vtprotobuf/features/equal        [no test files]
?       github.com/planetscale/vtprotobuf/features/grpc [no test files]
?       github.com/planetscale/vtprotobuf/features/marshal      [no test files]
?       github.com/planetscale/vtprotobuf/features/pool [no test files]
?       github.com/planetscale/vtprotobuf/features/size [no test files]
?       github.com/planetscale/vtprotobuf/features/unmarshal    [no test files]
?       github.com/planetscale/vtprotobuf/generator     [no test files]
?       github.com/planetscale/vtprotobuf/generator/pattern     [no test files]
?       github.com/planetscale/vtprotobuf/testproto/empty       [no test files]
?       github.com/planetscale/vtprotobuf/protohelpers  [no test files]
ok      github.com/planetscale/vtprotobuf/conformance   0.270s
?       github.com/planetscale/vtprotobuf/testproto/grpc        [no test files]
?       github.com/planetscale/vtprotobuf/testproto/grpc/inner  [no test files]
?       github.com/planetscale/vtprotobuf/testproto/ignore_unknown_fields       [no test files]
?       github.com/planetscale/vtprotobuf/testproto/proto2      [no test files]
ok      github.com/planetscale/vtprotobuf/conformance/internal/conformance      0.311s
?       github.com/planetscale/vtprotobuf/types/known/anypb     [no test files]
?       github.com/planetscale/vtprotobuf/types/known/durationpb        [no test files]
?       github.com/planetscale/vtprotobuf/types/known/emptypb   [no test files]
?       github.com/planetscale/vtprotobuf/types/known/fieldmaskpb       [no test files]
?       github.com/planetscale/vtprotobuf/types/known/structpb  [no test files]
?       github.com/planetscale/vtprotobuf/types/known/timestamppb       [no test files]
?       github.com/planetscale/vtprotobuf/types/known/wrapperspb        [no test files]
?       github.com/planetscale/vtprotobuf/vtproto       [no test files]
ok      github.com/planetscale/vtprotobuf/testproto/pool        0.232s
ok      github.com/planetscale/vtprotobuf/testproto/proto3opt   0.432s
ok      github.com/planetscale/vtprotobuf/testproto/unique      0.802s
ok      github.com/planetscale/vtprotobuf/testproto/unsafe      0.609s
ok      github.com/planetscale/vtprotobuf/testproto/wkt 1.030s
go test -count=1 ./conformance/...
ok      github.com/planetscale/vtprotobuf/conformance   1.122s
ok      github.com/planetscale/vtprotobuf/conformance/internal/conformance      0.447s
go test -count=1 ./testproto/ignore_unknown_fields/...
?       github.com/planetscale/vtprotobuf/testproto/ignore_unknown_fields       [no test files]
GOGC="off" go test -count=1 ./testproto/pool/...
ok      github.com/planetscale/vtprotobuf/testproto/pool        0.280s

Copy link
Member

@vmg vmg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you all these changes look correct. Good catch on the test assertion! 😓

@vmg
Copy link
Member

vmg commented Nov 21, 2024

@coxley: it seems like you've checked in conformance/marshal.log which is failing the tests. Is this is a mistake?

@coxley
Copy link
Contributor Author

coxley commented Nov 21, 2024

@vmg: whoops, should we add it to gitignore? I just ran the make targets.

@vmg
Copy link
Member

vmg commented Nov 21, 2024

Yeah let's do that. Thanks!

@coxley
Copy link
Contributor Author

coxley commented Nov 21, 2024

Done!

@vmg vmg merged commit 79df5c4 into planetscale:main Nov 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

features/unmarshal_unique: Maps with string-keys
2 participants