-
Notifications
You must be signed in to change notification settings - Fork 51
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.
Changes look good. Does it make sense to add a system test for this?
We already had a system test: https://github.com/GoogleCloudPlatform/cloud-debug-nodejs/blob/master/system-test/test-controller.js#L98 which didn't have the typo :(. I think a more comprehensive system test that actually sets a breakpoint would be needed. |
Adjusted the e2e test to better exercise waitExpired. Also fix |
@@ -197,13 +197,13 @@ describe('@google-cloud/debug end-to-end behavior', function () { | |||
'transcript in child ' + index + ' should contain value of o: ' + | |||
child.transcript); | |||
}); | |||
return api.deleteBreakpoint(debuggeeId, breakpoint.id); | |||
return Promise.resolve(); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
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 w/ question.
This was causing breakpoints to be locally expired incorrectly.