Skip to content

core: add strongly-typed ServiceConfigProto class for test #4369

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

core: add strongly-typed ServiceConfigProto class for test #4369

wants to merge 4 commits into from

Conversation

dapengzhang0
Copy link
Member

Add service_config.proto in test module.
This can generate strongly-typed service config object for testing.
The added test did find a bug that some keys in the proto did not match the json keys in the spec (b/78345359).

@dapengzhang0 dapengzhang0 force-pushed the serviceconfigtest branch 2 times, most recently from f8df92e to 51e2873 Compare April 20, 2018 20:42
@carl-mastrangelo
Copy link
Contributor

Not sure how I feel about this. SC was intentionally designed to have full flexibility in representation, with the good and the bad. By making our tests not match what real data will be sent in, we run the risk of blinding ourselves.

I understand this is test only code, and it would make testing easier. I don't know if it is a long term solution, as it doesn't match gRFC semantics. It would be nice to have static runtime checks that the data is valid, though.

@dapengzhang0
Copy link
Member Author

@carl-mastrangelo I agree that SC was intentionally designed to have full flexibility in representation,
but validating an object or ability to generate ALL possible valid objects is by no means the goal of this PR.
This PR only helps to generate valid objects for tests. It can not generate valid object with unknown keys, so it is not supposed to be used for all kinds of tests. This PR also helps to find typos for the known key constants in ServiceConfigUtil.java if there were any, but this PR does not test the correctness of ServiceConfigUtil or ServiceConfigInterceptor.

@dapengzhang0 dapengzhang0 force-pushed the serviceconfigtest branch 2 times, most recently from d83a9f9 to f368434 Compare April 20, 2018 22:37
ejona86 added a commit to ejona86/grpc-java that referenced this pull request Apr 21, 2018
@ejona86
kokoro: Avoid --include-build for Android
94ee5da 
Since 4369e8c the --include-build just opens us up to trouble with
accidentally building the protoc plugin. Since we're going to do a
./gradlew install anyway, let's just wait until after that point for
building cronet.

This sort of problem was experienced while developing grpc#4369.
@ejona86 ejona86 mentioned this pull request Apr 21, 2018
Merged
dapengzhang0 pushed a commit to dapengzhang0/grpc-java that referenced this pull request Apr 21, 2018
@ejona86 @dapengzhang0
kokoro: Avoid --include-build for Android
f97a853 
Since 4369e8c the --include-build just opens us up to trouble with
accidentally building the protoc plugin. Since we're going to do a
./gradlew install anyway, let's just wait until after that point for
building cronet.

This sort of problem was experienced while developing grpc#4369.
ejona86 added a commit that referenced this pull request Apr 21, 2018
@ejona86
kokoro: Avoid --include-build for Android
a47266e 
Since 4369e8c the --include-build just opens us up to trouble with
accidentally building the protoc plugin. Since we're going to do a
./gradlew install anyway, let's just wait until after that point for
building cronet.

This sort of problem was experienced while developing #4369.
ejona86
ejona86 reviewed Apr 23, 2018
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I'm fine with using protobuf to construct the JSON, but I don't see what the gain as written is; the code seems like it would have been clearer with just big JSON literals. It looks like there's a drive to avoid duplication, but it seems to have a negative impact to the test clarity.

core/src/test/proto/grpc/service_config.proto Outdated
@@ -0,0 +1,227 @@
// Copyright 2015, gRPC Authors
Copy link
Member

Choose a reason for hiding this comment

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

Where did this file come from?

core/src/test/java/io/grpc/internal/ServiceConfigUtilTest.java Outdated

Name.Builder name1 = Name.newBuilder().setService("fooService1").setMethod("barMethod1.1");
methodConfigBuilder.addName(name1);
expectedNames.add(protobufToJson(name1));
Copy link
Member

Choose a reason for hiding this comment

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

I see this pattern frequently here. But forming up input and expected values simultaneously makes it harder to see what both are. Duplicating the contents would probably be much more clear and would actually be shorter. Copying+pasting between input and output is not a bad thing for tests.

core/src/test/java/io/grpc/internal/ServiceConfigUtilTest.java Outdated
public class ServiceConfigUtilTest {

@Test
public void getMethodConfigFromServiceConfig() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this method so huge? Isn't it just testing a basic getter? The method being tested doesn't even care about the contents of the individual configs. It seems like it should just have a simple assertSame() and mostly ignore the configs themselves.

core/src/test/java/io/grpc/internal/ServiceConfigUtilTest.java Outdated
// TODO(zdapeng): getRetryThrottling from service config

@Test
public void getNameListFromMethodConfig() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

This seems unnecessarily long as well.

core/src/test/java/io/grpc/internal/ServiceConfigUtilTest.java Outdated

@Test
public void getMethodFromName() throws Exception {
String service = "fooService";
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a variable?

core/src/test/java/io/grpc/internal/ServiceConfigUtilTest.java Outdated
@Test
public void getMethodFromName() throws Exception {
String service = "fooService";
String method = "barMethod";
Copy link
Member

Choose a reason for hiding this comment

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

Inlining this seems like it'd be so much more clear:

Map<String, Object> name1Json = protobufToJson(
    Name.newBuilder().setService("fooService").setMethod("barMethod"));
assertEquals("barMethod", ServiceConfigUtil.getMethodFromName(name1Json));

Just to point out, the JSON version seems pretty obvious:

Map<String, Object> name1Json = parse("{\"service\":\"fooService\",\"method\":\"barMethod\"}");
assertEquals("barMethod", ServiceConfigUtil.getMethodFromName(name1Json));

All the backslashes are annoying; I'd probably make parse() include a replace("'", "\"") so that I could write the literal as "{'service':'fooService','method':'barMethod'}".

Copy link
Member Author

Choose a reason for hiding this comment

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

Will inline them.

These (proto version and JSON version) are two different tests. The former is to guarantee ServiceConfigUtil consistent with service_config.proto, whereas the latter emphasizes on logic correctness of ServiceConfigUtil.

Maybe I should rename the test class to ServiceConfigProtoConsistencyTest

Copy link
Member

Choose a reason for hiding this comment

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

:-/. Do we really need per-field checks for consistency between proto and JSON? Especially given that isn't a "blessed" proto. Also, it's not clear that's actually what the point of this test was. Why wouldn't the test have only set the method; why did it bother setting the service name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise what test would find inconsistency like b/78345359 ?

Copy link
Member

Choose a reason for hiding this comment

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

So should that test be internal then? I see no reason to believe this proto would be kept up-to-date, as nothing uses it. I'd be happy to have a proto schema that would be deemed equivalent to the JSON spec, but at present this proto isn't it (and would need modifications before that was possible).

Also, again, the tests are doing lots of things more than checking whether the field names match. It isn't at all clear what is being tested.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. Removed the test.

@dapengzhang0
Copy link
Member Author

closing the PR because it's not convincing that service config proto is helpful for test.

@dapengzhang0 dapengzhang0 deleted the serviceconfigtest branch May 2, 2018 17:15
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ejona86 ejona86 ejona86 left review comments

@carl-mastrangelo carl-mastrangelo Awaiting requested review from carl-mastrangelo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dapengzhang0 @carl-mastrangelo @ejona86