-
Notifications
You must be signed in to change notification settings - Fork 113
Conversation
@alexander-fenster please take a look |
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.
LGTM: with @alexander-fenster's approval.
@@ -211,6 +211,46 @@ describe('Compute', () => { | |||
}); | |||
assert.strictEqual(fetched.shieldedInstanceConfig.enableSecureBoot, true); | |||
}); | |||
|
|||
it('API errors propagated', async () => { |
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.
nit: I like for the assertions to read like a phrase.
it('propagates API errors')
.
instance: INSTANCE_NAME, | ||
instanceResource: instance, | ||
}); | ||
await waitZonalOperation(updateOp); |
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.
I like the use of async
/await
in tests 👍
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.
I should say, LGTM, with @summer-ji-eng or @alexander-fenster's approval.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕