tests: update tests to use LRO #587
Conversation
product-auto-label
bot
added
the
api: compute
| @@ -58,7 +56,7 @@ public void setUpMethod() { | |||
| @AfterClass | |||
| 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.
| @@ -65,7 +73,16 @@ public static void tearDown() { | |||
|
|
|||
| @Test | |||
| 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
Update tests to use LRO.
Also manually checked LRO cancel()