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

test(pd): add integration test for grpc reflection #4426

Merged
merged 1 commit into from
May 23, 2024

Conversation

conorsch
Copy link
Contributor

Describe your changes

Refs #4392. We want to ensure that server reflection remains working, despite changes to the tonic dependencies (#4400) and proto compiling logic #4422. While the most effective test is exercising all these protos regularly, we currently lack solid coverage, so this superficial integration test is a sanity-check to save time versus building locally and inspecting the output manually.

Issue ticket number and link

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    tests-only

@conorsch
Copy link
Contributor Author

Draft because I cobbled this together as part of initial work on #4392, and it proved useful while reviewing #4424 & #4425. Still needs some attention in getting CI working, and also pulling in the FILE_DESCRIPTOR set from the proto crate. Will circle back on it opportunistically next time I'm waiting on a long build.

@conorsch conorsch requested a review from cratelyn May 21, 2024 01:46
Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

i am in deep, earnest support of this! 🙌 a tested feature is a non-regressing feature. thank you for starting this work @conorsch!

@conorsch conorsch force-pushed the 4392-regression-test-for-reflection branch from 08f60ec to ad9a444 Compare May 23, 2024 00:20
@conorsch conorsch force-pushed the 4392-regression-test-for-reflection branch from ad9a444 to 850c973 Compare May 23, 2024 00:26
@conorsch conorsch force-pushed the 4392-regression-test-for-reflection branch from 850c973 to 46ab3e8 Compare May 23, 2024 15:40
@conorsch conorsch marked this pull request as ready for review May 23, 2024 15:54
@@ -28,6 +28,9 @@ jobs:
sh -c "$(curl --location https://raw.githubusercontent.com/F1bonacc1/process-compose/main/scripts/get-pc.sh)" --
-d -b ~/bin

- name: Install grpcurl
run: ./deployments/scripts/install-grpcurl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cratelyn N.B. I'd much rather use something like nix flakes or devbox to manage these dependencies. Not going down that rabbit hole now, but flagging that it'd be less work overall to track tooling deps in a holistic way, cf. #4264.

@conorsch conorsch requested a review from cratelyn May 23, 2024 16:07
Refs #4392. We want to ensure that server reflection remains
working, despite changes to the tonic dependencies (#4400)
and proto compiling logic #4422. While the most effective test
is exercising all these protos regularly, we currently lack
solid coverage, so this superficial integration test is a sanity-check
to save time versus building locally and inspecting the output manually.
@conorsch conorsch force-pushed the 4392-regression-test-for-reflection branch from 46ab3e8 to 711494d Compare May 23, 2024 16:23
Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

🧪 🎉

@conorsch conorsch merged commit 299629b into main May 23, 2024
13 checks passed
@conorsch conorsch deleted the 4392-regression-test-for-reflection branch May 23, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants