-
Notifications
You must be signed in to change notification settings - Fork 107
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
Run conformance tests in CI #642
Conversation
8f42805
to
c6c3c62
Compare
c6c3c62
to
30c33fa
Compare
codecs: | ||
- CODEC_PROTO | ||
- CODEC_JSON | ||
- CODEC_TEXT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, Content-Type: application/text
for unary Connect-protocol RPCs is a little rough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to do here, just an idle comment on the not-great content-type we end up with. It's nice that we can test this in the conformance suite, though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's true. I mainly added to the conformance tests because it is a codec that is supported by default by the grpc-web JS stuff (since they can't support JSON because they're using a protobuf runtime w/out JSON support, that is the codec to use for human-readable payloads).
Similarly, all the compression algorithms listed are ones that are actually referenced in the Connect wire protocol spec (br, zstd) or the gRPC HTTP/2 protocol spec (deflate, snappy). So if someone implements them, it seemed reasonable to be able to check them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grpc/grpc-web clients use either protobuf binary, or protobuf binary with and additional base64 encoding on top, which they call grpc-web-text. They do not use the protobuf text format, which I believe this is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the protobuf text format. It's very different from the grpc-web-text base64-encoding stuff, which does not produce human readable messages in the network inspector.
I can't seem to find the support for the text codec anymore. Maybe it was removed. At FullStory, we investigated using grpc-web circa 2017/2018. IIRC, that was before there was an official one in the grpc GitHub org, so we were looking at the TS stuff in improbable-eng/grpc-web. We experimented with using the text codec. (I remember having to configure our gRPC servers to support it, since it is not a supported codec out-of-the-box in grpc-go. I also remember being very confused and frustrated that there was no support in protobuf-js for JSON.)
So it may not be present anymore, in either the official implementation or the improbable-eng one. But I had added it as a supported codec in the conformance tests from my memory of that. We can always remove it if we think no one would ever use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how the text format would have looked like a good option back then. Doesn't seem to hurt to test it. It is definitely not used anywhere I know with gRPC-Web.
This uses the newest updates in the connectrpc/conformance repo to run conformance tests as part of CI, with every PR and every commit to main.
Since the conformance repo's reference client and reference server were written using connect-go, we are simply leveraging that implementation in the conformance repo. And we use a
go.mod
file with a replace directive to effectively build the conformance tests and the reference client and server using the current HEAD of this repo.