-
Notifications
You must be signed in to change notification settings - Fork 59
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: Integration test close function again #1103
fix: Integration test close function again #1103
Conversation
…into integration-test-close-function-again # Conflicts: # src/table.ts
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.
A comment to clarify the original problem.
@@ -930,7 +930,7 @@ export class Bigtable { | |||
* that a callback is omitted. | |||
*/ | |||
promisifyAll(Bigtable, { | |||
exclude: ['instance', 'operation', 'request'], | |||
exclude: ['close', 'instance', 'operation', 'request'], |
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.
In the absence of this change, the test just seems to hang forever.
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.
promisifyAll
turns a cb
based function into a promise based function, it looks like close was already a promise, and did not expect a callback
as an argument:
close(): Promise<void[]>
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.
Yes. I am making sure the reader of this PR knows why this change is necessary.
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.
Thanks for clarifying why the test hangs in this PR though!
@@ -930,7 +930,7 @@ export class Bigtable { | |||
* that a callback is omitted. | |||
*/ | |||
promisifyAll(Bigtable, { | |||
exclude: ['instance', 'operation', 'request'], | |||
exclude: ['close', 'instance', 'operation', 'request'], |
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.
promisifyAll
turns a cb
based function into a promise based function, it looks like close was already a promise, and did not expect a callback
as an argument:
close(): Promise<void[]>
This fixes an issue with the close function where promises of promises were once returned, but now we simply return a promise so that when we use
await
then the code is executed properly.