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

Implement unary operations in conformance client #195

Merged
merged 1 commit into from
Jan 11, 2024
Merged

Conversation

jhump
Copy link
Member

@jhump jhump commented Jan 10, 2024

This also adds support for other command-line args, to enable verbose tracing output (which was handy for troubleshooting stuff).

Other repos use the make target runconformance, but this repo was using conformancerun, so I changed it. That target still runs the old cross test stuff, but it also now runs these new unary conformance test cases. The config file is what indicates unary-only. The "known failing" file is because the assertions in the test runner for Connect GET requests are too strict -- AFAICT, this client now passes those tests. The only failing assertion is about the message query param that the server gets because it is expecting the encoding in a particular way, but that is not necessary (but requires a change in the test runner to relax the assertion).

The Makefile downloads the conformance test runner in the same fashion as it downloads protoc.

@mkdir -p $(dir $@)
@touch $@
Copy link
Member Author

Choose a reason for hiding this comment

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

This seemed weird that it would create an empty placeholder zip file. So I tweaked this so that the artifact it creates is the actual downloaded ZIP file.

I also added a distinct target for the .tmp/bin/protoc binary itself because, TBH, I found it very confusing that the $(PROTOC) target was actually referencing a bogus ZIP file.

Copy link
Contributor

Choose a reason for hiding this comment

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

At some point I'd love to move all of this into the gradle build so we only have to maintain a single build system but this is a nice improvement.

…o that unary conformance tests are run during CI
@mkdir -p $(dir $@)
@touch $@
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point I'd love to move all of this into the gradle build so we only have to maintain a single build system but this is a nice improvement.

}
}
}
return ClientArgs(invokeStyle, verbosity)
Copy link
Contributor

Choose a reason for hiding this comment

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

This works for now but in the future I've had good success with https://args4j.kohsuke.org/ if we need more complexity here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've used a similar library, JCommander. I actually briefly looked into Clickt, which seems to be the most popular library for Kotlin command-line apps. But I had an issue with it (likely something quirky in the IDE, though I did update the project after adding a dep in gradle, but it still wouldn't work), so I decided to punt on pulling in a dep for this, since this logic is so simple 🤷

) {
fun run(input: InputStream, output: OutputStream, client: Client) = runBlocking {
// TODO: issue RPCs in parallel
while (true) {
var result: ClientCompatResponse.Result
val req = readRequest(input) ?: return@runBlocking // end of stream
if (verbosity > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another option for more debugging might be to configure the okhttp logging interceptor (perhaps at higher verbosity?).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack. That would likely be much more verbose than this. I haven't needed that sort of output so far, but it does seem like a potentially useful idea.

@jhump jhump merged commit 8ecb275 into main Jan 11, 2024
7 checks passed
@jhump jhump deleted the jh/unary-impl branch January 11, 2024 14:40
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