Skip to content

Move unit tests to local protos #288

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

Merged
merged 4 commits into from
Apr 21, 2025
Merged

Move unit tests to local protos #288

merged 4 commits into from
Apr 21, 2025

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented Apr 18, 2025

The unit tests in this repo use Protobuf definitions from the conformance suite, which works but it makes debugging issues a bit difficult. It is extremely helpful to have local protos in the repo that can be changed for tests to debug issues.

This follows the example in protovalidate-go by creating a validations.proto and copies the definitions currently being used from conformance into this proto.

Comment on lines +36 to +38
$(BIN)/buf generate buf.build/bufbuild/protovalidate:$(PROTOVALIDATE_VERSION)
$(BIN)/buf generate buf.build/bufbuild/protovalidate-testing:$(PROTOVALIDATE_VERSION)
$(BIN)/buf generate
Copy link
Member

Choose a reason for hiding this comment

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

I agree that it's helpful to have local proto files for debugging. But PROTOVALIDATE_VERSION and buf.lock competing over the exact version seems like a footgun.

Maybe the least controversial thing to do is to cut a protovalidate release that contains the new test protos?

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m not following. pv-go is doing this exact thing with no issues. I would really prefer for this to follow that pattern. Being able to change protos while debugging issues is much quicker than constantly having to cut protovalidate releases.

Copy link
Member

Choose a reason for hiding this comment

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

It's good that we haven't had an issue with it so far. I didn't realize the Go implementation already does it. LGTM, and hopefully find a better solution down the road.

@smaye81 smaye81 merged commit 982f604 into main Apr 21, 2025
13 checks passed
@smaye81 smaye81 deleted the sayers/add_local_protos branch April 21, 2025 14:43
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.

2 participants