Skip to content
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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jul 12, 2022
Merged

fix: Integration test close function again #1103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
60a98a5
First try
danieljbruce Apr 25, 2022
ca563b7
Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable …
danieljbruce May 30, 2022
79b1087
Add after hook
danieljbruce Jun 20, 2022
90c919f
Proper test done for closed client
danieljbruce Jun 20, 2022
b2bb4ae
test description change
danieljbruce Jun 20, 2022
422c0c9
Eliminate unnecessary comment
danieljbruce Jun 20, 2022
037fa65
Modify test to reflect change
danieljbruce Jun 20, 2022
ad0d59f
Merge branch 'main' into integration-test-close-function-again
danieljbruce Jun 20, 2022
f8d0980
Merge branch 'main' into integration-test-close-function-again
danieljbruce Jun 21, 2022
94b48b1
Merge branch 'main' into integration-test-close-function-again
danieljbruce Jun 22, 2022
066ae87
Merge branch 'main' into integration-test-close-function-again
danieljbruce Jul 11, 2022
f3fb944
Merge branch 'main' into integration-test-close-function-again
danieljbruce Jul 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,7 @@ export class Bigtable {
* that a callback is omitted.
*/
promisifyAll(Bigtable, {
exclude: ['instance', 'operation', 'request'],
exclude: ['close', 'instance', 'operation', 'request'],
Copy link
Contributor Author

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.

Copy link
Contributor

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[]>

Copy link
Contributor Author

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.

Copy link
Contributor Author

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!

});

/**
Expand Down
33 changes: 32 additions & 1 deletion system-test/read-rows.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {describe, it, afterEach, beforeEach} from 'mocha';
import * as sinon from 'sinon';
import {EventEmitter} from 'events';
import {Test} from './testTypes';
import {ServiceError, GrpcClient} from 'google-gax';
import {ServiceError, GrpcClient, GoogleError} from 'google-gax';
import {PassThrough} from 'stream';

const {grpc} = new GrpcClient();
Expand Down Expand Up @@ -77,12 +77,43 @@ function rowResponse(rowKey: {}) {

describe('Bigtable/Table', () => {
const bigtable = new Bigtable();
const INSTANCE_NAME = 'fake-instance2';
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(bigtable as any).grpcCredentials = grpc.credentials.createInsecure();

const INSTANCE = bigtable.instance('instance');
const TABLE = INSTANCE.table('table');

describe('close', () => {
it('should fail when invoking readRows with closed client', async () => {
const instance = bigtable.instance(INSTANCE_NAME);
const table = instance.table('fake-table');
await instance.create({
clusters: {
id: 'fake-cluster3',
location: 'us-west1-c',
nodes: 1,
},
});
await table.create({});
await table.getRows(); // This is done to initialize the data client
await bigtable.close();
try {
await table.getRows();
assert.fail(
'An error should have been thrown because the client is closed'
);
} catch (err: any) {
assert.strictEqual(err.message, 'The client has already been closed.');
}
});
after(async () => {
const bigtableSecondClient = new Bigtable();
const instance = bigtableSecondClient.instance(INSTANCE_NAME);
await instance.delete({});
});
});

describe('createReadStream', () => {
let clock: sinon.SinonFakeTimers;
let endCalled: boolean;
Expand Down
1 change: 1 addition & 0 deletions test/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const fakePromisify = Object.assign({}, promisify, {
}
promisified = true;
assert.deepStrictEqual(options.exclude, [
'close',
'instance',
'operation',
'request',
Expand Down