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

Make the OkHttp and Netty tests actually use OkHttp/Netty #928

Merged
merged 3 commits into from
Jun 4, 2020

Conversation

cb372
Copy link
Member

@cb372 cb372 commented Jun 4, 2020

grpc-java chooses the client implementation by finding all Providers
on the classpath and picking the one with the highest priority. The
tests module has both grpc-netty and grpc-okhttp on the classpath,
so it will always choose the Netty implementation unless we specify
otherwise.

Added a couple of parameters to ManagedChannelInterpreter's
constructor to allow us to inject the appropriate implementation in
tests, and updated the tests accordingly.

What this does?

Changes, features, fixes ...

Checklist

  • Reviewed the diff to look for typos, println and format errors.
  • Updated the docs accordingly.

grpc-java chooses the client implementation by finding all `Provider`s
on the classpath and picking the one with the highest priority. The
`tests` module has both `grpc-netty` and `grpc-okhttp` on the classpath,
so it will always choose the Netty implementation unless we specify
otherwise.

Added a couple of parameters to `ManagedChannelInterpreter`'s
constructor to allow us to inject the appropriate implementation in
tests, and updated the tests accordingly.
@codecov
Copy link

codecov bot commented Jun 4, 2020

Codecov Report

Merging #928 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #928      +/-   ##
==========================================
+ Coverage   88.70%   88.71%   +0.01%     
==========================================
  Files          58       58              
  Lines         832      833       +1     
  Branches        3        2       -1     
==========================================
+ Hits          738      739       +1     
  Misses         94       94              
Impacted Files Coverage Δ
...ess/mu/rpc/channel/ManagedChannelInterpreter.scala 90.90% <100.00%> (+0.90%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcbf59d...4190187. Read the comment docs.

configList: List[ManagedChannelConfig]
configList: List[ManagedChannelConfig],
builderForAddress: (String, Int) => ManagedChannelBuilder[_] = ManagedChannelBuilder.forAddress,
builderForTarget: String => ManagedChannelBuilder[_] = ManagedChannelBuilder.forTarget
Copy link
Member

Choose a reason for hiding this comment

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

Would be this a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is source-compatible because of the default arguments, but it is binary-incompatible: https://github.com/jatcwang/binary-compatibility-guide#dont-adding-parameters-with-default-values-to-methods

I'll leave the primary constructor alone and add a secondary constructor for use in tests.

)(implicit F: Sync[F]) {

// Secondary constructor added for bincompat
def this(
Copy link
Member Author

Choose a reason for hiding this comment

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

Checked locally using MiMa that this fixes the bincompat breakage

@cb372 cb372 merged commit 47f7a6b into master Jun 4, 2020
@cb372 cb372 deleted the okhttp-client-tests branch June 4, 2020 15:46
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.

3 participants