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

Refactoring of gRPC internals & tests and introducing the reflectMetadata #3343

Merged
merged 4 commits into from
Sep 22, 2023

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Sep 20, 2023

What?

This PR contains several parts:

  • First is refactoring of calling and connection params to make code more re-usable
  • Second is the significant (but is mostly moving & cleaning) refactoring of the gRPC client's test
  • last but not least is an introduction to the reflectMetadata

Why?

The refactoring was done to make the following changes tinier and pay some technical dept of removing copy-pasting.

The reflectMetadata was requested in #3241

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

grafana/xk6-grpc#51

Closes: #3241

Changelog

epic-feature: a new gRPC connection's parameter reflectMetadata

In some workflows, the reflection call should also include some metadata. This PR adds a new connection parameter reflectMetadata that allows to specify the metadata to be sent with the reflection call.

@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2023

Codecov Report

Patch coverage: 66.66% and no project coverage change.

Comparison is base (b8a6ecf) 73.17% compared to head (ce3833f) 73.17%.

❗ Current head ce3833f differs from pull request most recent head f100921. Consider uploading reports for the commit f100921 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3343   +/-   ##
=======================================
  Coverage   73.17%   73.17%           
=======================================
  Files         258      258           
  Lines       19886    19903   +17     
=======================================
+ Hits        14551    14564   +13     
- Misses       4414     4416    +2     
- Partials      921      923    +2     
Flag Coverage Δ
ubuntu 73.11% <66.66%> (+<0.01%) ⬆️
windows 73.03% <66.66%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
js/modules/k6/grpc/client.go 83.38% <66.66%> (-0.98%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mstoykov
mstoykov previously approved these changes Sep 20, 2023
@olegbespalov olegbespalov added this to the v0.47.0 milestone Sep 20, 2023
@olegbespalov
Copy link
Contributor Author

I rebased, since there was a minor conflict with master branch.

@@ -1,16 +1,11 @@
package grpc
package grpc_test
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why are we moving them to a specific package? Do we have cycle dependencies or other issues? I'm not against it, but I've seen some ongoing discussions around gRPC and extensions and I have the feeling I lost something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say it's just a better way of structuring the tests (best practice), basically test package becomes a user of the main package, which blocks access to the main package's internals, and as a result, we should write more robust tests, since even something internally in the package changed the tests continue to works since they rely on the public API

@@ -1325,74 +1131,44 @@ func TestDebugStat(t *testing.T) {
func TestClientInvokeHeadersDeprecated(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like it could be the time to drop it completly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is still relevant for the existing functionality. However, I like the idea of dropping the headers property, but how about opening a separate PR where we start erroring (right now, we just fallthrough to the metadata) and keep this for a cycle/too till we altogether remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with it 👍

@olegbespalov olegbespalov merged commit 1892f91 into master Sep 22, 2023
21 checks passed
@olegbespalov olegbespalov deleted the backporting/reflectMetadata branch September 22, 2023 09:11
@olegbespalov olegbespalov added the documentation-needed A PR which will need a separate PR for documentation label Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: grpc documentation-needed A PR which will need a separate PR for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add metadata to reflection in grpc
4 participants