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

Moving the experimental grpc module to the core #3490

Merged
merged 4 commits into from
Dec 15, 2023

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Dec 5, 2023

What?

We are moving the experimental GRCP module to the core.

It is worth to review by commits, but in short:

Why?

The main feature of the experimental module (streaming) works as expected. We're ready to move forward.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make 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)

Closes #3152

@olegbespalov olegbespalov added this to the v0.49.0 milestone Dec 5, 2023
@olegbespalov olegbespalov self-assigned this Dec 5, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2023

Codecov Report

Attention: 93 lines in your changes are missing coverage. Please review.

Comparison is base (bfdc446) 73.21% compared to head (fc40970) 72.40%.

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

Files Patch % Lines
lib/testutils/grpcservice/service.go 68.08% 25 Missing and 5 partials ⚠️
...httpmultibin/grpc_wrappers_testing/test_grpc.pb.go 25.00% 26 Missing and 1 partial ⚠️
js/modules/k6/grpc/grpc.go 60.71% 14 Missing and 8 partials ⚠️
js/modules/k6/grpc/client.go 76.92% 3 Missing and 3 partials ⚠️
lib/netext/grpcext/conn.go 77.77% 2 Missing and 2 partials ⚠️
cmd/tests/grpc.go 90.47% 1 Missing and 1 partial ⚠️
...tils/httpmultibin/grpc_wrappers_testing/service.go 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3490      +/-   ##
==========================================
- Coverage   73.21%   72.40%   -0.82%     
==========================================
  Files         267      274       +7     
  Lines       20083    20847     +764     
==========================================
+ Hits        14704    15094     +390     
- Misses       4465     4782     +317     
- Partials      914      971      +57     
Flag Coverage Δ
ubuntu ?
windows 72.40% <64.09%> (-0.68%) ⬇️

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

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

@olegbespalov olegbespalov marked this pull request as ready for review December 5, 2023 18:38
@github-actions github-actions bot requested review from mstoykov and oleiade December 5, 2023 18:38
@olegbespalov olegbespalov marked this pull request as draft December 5, 2023 18:50
@olegbespalov olegbespalov marked this pull request as ready for review December 6, 2023 08:13
@@ -35,7 +34,7 @@ func getInternalJSModules() map[string]interface{} {
"k6/experimental/redis": redis.New(),
"k6/experimental/webcrypto": webcrypto.New(),
"k6/experimental/websockets": &expws.RootModule{},
"k6/experimental/grpc": expGrpc.New(),
"k6/experimental/grpc": grpc.NewExperimental(),
Copy link
Contributor

Choose a reason for hiding this comment

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

It kind of makes more sense to me to move the warning logic to this file only so that the grpc parts are left only about the actual implementation.

Also will be easier to remove as you need to modify only here.

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 assumed that this would be executed even if no module imports were in the script, no? 🤔 so it is either moving this logic to the module's resolver or...

On the other side, the clean up isn't hard and I even could sketch up a PR for a long term milestone for it

Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

👏🚀

@olegbespalov olegbespalov merged commit 4671aeb into master Dec 15, 2023
23 checks passed
@olegbespalov olegbespalov deleted the feat/graduating-grpc branch December 15, 2023 08:08
@mstoykov mstoykov added the documentation-needed A PR which will need a separate PR for documentation label Jan 15, 2024
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.

Making k6/experimental/grpc module part of the k6 core
4 participants