tests: update tests to use LRO#587
Conversation
product-auto-label
Bot
added
the
api: compute
vam-google
left a comment
vam-google
left a comment
There was a problem hiding this comment.
LGTM with few minor comments.
As for failing tests, I think is ok - this PR merges into a branch, not into a master. And the branch it tires to merge is broken already (this is kind of the point of this PR - to fix as much as possible that broken branch)
| public static void tearDown() { | ||
| for (Address address : addresses) { | ||
| addressesClient.delete(DEFAULT_PROJECT, DEFAULT_REGION, address.getName()); | ||
| addressesClient.deleteAsync(DEFAULT_PROJECT, DEFAULT_REGION, address.getName()); |
There was a problem hiding this comment.
I believe it is better wait for completion. Even though technically, we may quit and let the operation do its job on the server side, but waiting and potentially reporting deletion error seems cleaner and more robust.
| e.printStackTrace(); | ||
| } catch (TimeoutException e) {; // expected | ||
| } | ||
| future.cancel(true); |
There was a problem hiding this comment.
Is this by any chance testing operation cancellation functionality as well?
There was a problem hiding this comment.
Unfortunately it doesn't, I tested it manually. e2e test for this seems a little bit more complicated, had no time to work on that.
| public void testHeaders() { | ||
| addressesClient.insert("test", "test", Address.newBuilder().setName("test").build()); | ||
| OperationFuture<Operation, Operation> future = | ||
| addressesClient.insertAsync("test", "test", Address.newBuilder().setName("test").build()); |
There was a problem hiding this comment.
Please give params more descriptive values than "test" and "test", this is a good practice even if the values do not really matter for the test itself (increases the test code readability and is easy to do).
| fail("Did not catch the exception"); | ||
| } catch (InvalidArgumentException ex) { | ||
| String message = "Bad Request"; | ||
| } catch (ExecutionException ex) { |
There was a problem hiding this comment.
ExecutionException is caught here, but is also declared in throws block above. Do you really need both?
There was a problem hiding this comment.
We don't, removed from declaration
2 participants
Update tests to use LRO.
Also manually checked LRO cancel()