Added tests for gRPC retries. #69
Conversation
| @@ -140,6 +140,12 @@ | |||
| <artifactId>grpc-all</artifactId> | |||
| <version>0.9.0</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>io.grpc</groupId> | |||
| <artifactId>grpc-interop-testing</artifactId> | |||
There was a problem hiding this comment.
You really shouldn't depend on grpc-interop-testing; it's only there for people to run the interop clients if they want. It shouldn't be hard to remove the dependency of AbstractTransportTest.
Also tidy up visibility modifiers and ordering.
|
@ejona86 PTAL |
|
|
||
| protected static class VariantsIntegrationServerImpl implements | ||
| StreamingVariantServiceGrpc.StreamingVariantService { | ||
| final Random random = new Random(); |
There was a problem hiding this comment.
Seems like you should be using a seed here.
There was a problem hiding this comment.
Its intentional for these integration tests --> it will be a slightly different test every time but all runs should pass regardless of differences in the sequence of faults injected within the transfer.
I'm testing fixed sequences of faults in the unit tests.
Sound okay to you?
There was a problem hiding this comment.
If the probability of failure due to simulated failure is very low, I guess this is fine. I'd still prefer seeded random values if this is being run on the continuous integration.
There was a problem hiding this comment.
Travis CI is currently running the unit tests but not the integration tests. The integration tests are run as part of the release process and also on demand.
On a separate note, I had a flaky unit test because I was not adequately protecting the variables controlling the retry behavior from the channel and service threads interacting with them. With that fix, I think this PR is now good to go, but please let me know if there are any other changes you would like to see in this PR? Thanks!
| /** | ||
| * A convenience class for creating gRPC channels to the Google Genomics API. | ||
| */ | ||
| public class GenomicsChannel extends Channel { | ||
| public class GenomicsChannel extends ManagedChannel { |
There was a problem hiding this comment.
I wouldn't suggest you to implement ManagedChannel. You can keep the factory methods, but I would suggest they just return a ManagedChannel.
There was a problem hiding this comment.
I didn't see any public non-static methods. Did you make this class thinking you might add some in the future?
There was a problem hiding this comment.
Good question Eric. I made this class when we needed to expose shutdown for the hangs 042a406 ManagedChannel did not exist at that time, only ChanneImpl.
I'll refactor now - thanks.
There was a problem hiding this comment.
Oh, before you could add interceptors via the ChannelBuilder. That makes sense.
Now that we are using ManagedChannel instead of ChannelImpl.
|
@ejona86 Thanks for the feedback! Please let me know if there are any other changes you would like to see in this PR. |
|
@deflaux I didn't see anything else to point out. |
|
LGTM Sidenote: could we file an issue to have an automated style cleanup over this repo? There was a lot of noise added to this PR due to style-only changes (presumably from your IDE) which made this harder to review. |
|
Filed #70 |
The main code changed a bit to make it easier to pass in the channel created by the AbstractTransportTest but otherwise this is mostly new tests.
Added: