test(generator): add unit goldens for resumable upload stubs, client, and settings - #14323
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces tests and golden files for the new Resumable Upload feature in both gRPC and REST (HttpJson) stub and client generators. The review feedback highlights several code quality and efficiency issues in the generated golden files that should be addressed in the generator. These include double-escaped HTML tags in Javadocs, unused ProtoRestSerializer variables, inefficient empty collection instantiations, and split variable declarations.
| /** | ||
| * REST stub transport for the resumable upload methods of ResumableUploadService. | ||
| * | ||
| * <p><p>This class is for advanced usage and reflects the underlying API directly. |
There was a problem hiding this comment.
The generated Javadoc contains a double-escaped paragraph tag: <p><p>. This causes the literal text <p> to be rendered in the Javadoc HTML. The generator should be updated to produce a single, clean <p> tag (similar to the gRPC stub Javadoc generation).
* <p>This class is for advanced usage and reflects the underlying API directly.
| Map<String, String> fields = new HashMap<>(); | ||
| ProtoRestSerializer<UploadMediaRequest> serializer = | ||
| ProtoRestSerializer.create(); | ||
| return fields; |
There was a problem hiding this comment.
The serializer variable is instantiated but never used because there are no path parameters to serialize in this request formatter. The generator should be updated to omit generating the ProtoRestSerializer instantiation when it is not used, avoiding unused variable warnings and unnecessary object allocation.
Map<String, String> fields = new HashMap<>();
return fields;
| Map<String, List<String>> fields = new HashMap<>(); | ||
| ProtoRestSerializer<UploadMediaRequest> serializer = | ||
| ProtoRestSerializer.create(); | ||
| return fields; |
There was a problem hiding this comment.
The serializer variable is instantiated but never used because there are no query parameters to serialize in this query params extractor. The generator should be updated to omit generating the ProtoRestSerializer instantiation when it is not used, avoiding unused variable warnings and unnecessary object allocation.
Map<String, List<String>> fields = new HashMap<>();
return fields;
| static { | ||
| ImmutableMap.Builder<String, ImmutableSet<StatusCode.Code>> definitions = | ||
| ImmutableMap.builder(); | ||
| definitions.put("no_retry_codes", ImmutableSet.copyOf(Lists.<StatusCode.Code>newArrayList())); |
There was a problem hiding this comment.
Using ImmutableSet.copyOf(Lists.<StatusCode.Code>newArrayList()) to define an empty set is inefficient and verbose. The generator should be updated to output ImmutableSet.of() instead.
definitions.put("no_retry_codes", ImmutableSet.of());
| RetrySettings settings = null; | ||
| settings = RetrySettings.newBuilder().setRpcTimeoutMultiplier(1.0).build(); |
There was a problem hiding this comment.
The generated code declares RetrySettings settings = null; and then immediately assigns a value to it on the next line. The generator should be updated to combine these into a single statement to improve readability and code quality.
RetrySettings settings = RetrySettings.newBuilder().setRpcTimeoutMultiplier(1.0).build();
| List<ApiMethodDescriptor> methodDescriptors = new ArrayList<>(); | ||
| return methodDescriptors; | ||
| } |
There was a problem hiding this comment.
Instantiating a new ArrayList just to return it empty is inefficient. The generator should be updated to return an immutable empty list, such as java.util.Collections.emptyList(), when there are no method descriptors.
public static List<ApiMethodDescriptor> getMethodDescriptors() {
return java.util.Collections.emptyList();
}
|
|
1 participant
Work in progress, not yet ready for review