-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
fix(tpu): improving tpu tests #9679
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good work. thank you for finding the solution.
a few minor refactoring comments. plus a question regarding discrepancy of different runners used for different test classes
OperationFuture mockFuture = mock(OperationFuture.class); | ||
when(mockTpuClient.createNodeAsync(any(CreateNodeRequest.class))) | ||
.thenReturn(mockFuture); | ||
CreateTpuVm.createTpuVm( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add assert to verify that the return value is one that is expected to be returned by createNodeAsync()
call or whatever the code sample method returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are mocking TpuClient, we can expect that the method has been called but no resources have been created, so the return value of CreateTpuVm.createTpuVm() will be null. Based on this example
Mockito.mockStatic(ManagedKafkaClient.class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to print. To your argument, since you control the return value you can ensure that the client library returns whatever you like. It is exactly the same as with e2e testing when you expect the valid not null response and you validate vs that response.
Note that not all code samples return the object they get from calling a client library method "as is". Some return only a field and others do a sequence of calls.
OperationFuture mockFuture = mock(OperationFuture.class); | ||
when(mockTpuClient.createQueuedResourceAsync(any(CreateQueuedResourceRequest.class))) | ||
.thenReturn(mockFuture); | ||
CreateQueuedResourceWithNetwork.createQueuedResourceWithNetwork( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add assert to verify that the return value is one that is expected to be returned by createQueuedResourceAsync()
call or whatever the code sample method returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are mocking TpuClient, we can expect that the method has been called but no resources have been created, so the return value of CreateQueuedResourceWithNetwork.createQueuedResourceWithNetwork() will be null. Based on this example
Mockito.mockStatic(ManagedKafkaClient.class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my response in another comment thread
when(mock(TpuClient.class) | ||
.getQueuedResource(any(QueuedResourceName.class))).thenReturn(mockQueuedResource); | ||
|
||
GetQueuedResource.getQueuedResource(PROJECT_ID, ZONE, NODE_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add assert to verify that the return value is one that is expected to be returned by getQueuedResource()
call or whatever the code sample method returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are mocking TpuClient, we can expect that the method has been called but no resources have been created before, so the return value of GetQueuedResource.getQueuedResource() will be null. Based on this example
Mockito.mockStatic(ManagedKafkaClient.class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my response in another comment thread
|
||
Node node = CreateTpuVm.createTpuVm( | ||
PROJECT_ID, ZONE, NODE_NAME, TPU_TYPE, TPU_SOFTWARE_VERSION); | ||
GetTpuVm.getTpuVm(PROJECT_ID, ZONE, NODE_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add assert to verify that the return value is one that is expected to be returned by getNode()
call or whatever the code sample method returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are mocking TpuClient, we can expect that the method has been called but no resources have been created before, so the return value of GetTpuVm.getTpuVm() will be null. Based on this example
Mockito.mockStatic(ManagedKafkaClient.class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my response in another comment thread
Description
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
pom.xml
parent set to latestshared-configuration
mvn clean verify
requiredmvn -P lint checkstyle:check
requiredmvn -P lint clean compile pmd:cpd-check spotbugs:check
advisory only