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.

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

test: rewrite createreadstream tests #1401

Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
1 change: 0 additions & 1 deletion src/util/mock-servers/mock-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ export class MockServer {
`localhost:${this.port}`,
grpc.ServerCredentials.createInsecure(),
() => {
server.start();
Copy link
Contributor Author

@danieljbruce danieljbruce Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now deprecated. grpc servers don't need to be started explicitly.

callback ? callback(portString) : undefined;
}
);
Expand Down
269 changes: 134 additions & 135 deletions system-test/read-rows.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,69 +12,76 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import {Bigtable} from '../src';
import {Mutation} from '../src/mutation.js';
import {Bigtable, protos, Table} from '../src';
const {tests} = require('../../system-test/data/read-rows-retry-test.json') as {
tests: Test[];
tests: ReadRowsTest[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Test interface responses actually don't line up with what ReadRow responses should be so we define new interfaces that correctly match the structure of this data.

};
import {google} from '../protos/protos';
import * as assert from 'assert';
import {describe, it, afterEach, beforeEach} from 'mocha';
import * as sinon from 'sinon';
import {EventEmitter} from 'events';
import {Test} from './testTypes';
import {ServiceError, GrpcClient, GoogleError, CallOptions} from 'google-gax';
import {PassThrough} from 'stream';
import {describe, it, before} from 'mocha';
import {ReadRowsTest} from './testTypes';
import {ServiceError, GrpcClient, CallOptions} from 'google-gax';
import {MockServer} from '../src/util/mock-servers/mock-server';
import {MockService} from '../src/util/mock-servers/mock-service';
import {BigtableClientMockService} from '../src/util/mock-servers/service-implementations/bigtable-client-mock-service';
import {ServerWritableStream} from '@grpc/grpc-js';

const {grpc} = new GrpcClient();

// eslint-disable-next-line @typescript-eslint/no-explicit-any
function dispatch(emitter: EventEmitter, response: any) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const emits: any[] = [{name: 'request'}];
if (response.row_keys) {
emits.push.apply(emits, [
{name: 'response', arg: 200},
{
name: 'data',
arg: {chunks: response.row_keys.map(rowResponse)},
},
]);
}
if (response.end_with_error) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const error: any = new Error();
error.code = response.end_with_error;
emits.push({name: 'error', arg: error});
} else {
emits.push({name: 'end'});
}
let index = 0;
setImmediate(next);

function next() {
if (index < emits.length) {
const emit = emits[index];
index++;
emitter.emit(emit.name, emit.arg);
setImmediate(next);
}
}
}

function rowResponse(rowKey: {}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A response from the server is modified slightly to match what a response would be from the request layer which explains the changes in this function.

function rowResponseFromServer(rowKey: string) {
return {
rowKey: Mutation.convertToBytes(rowKey),
rowKey: Buffer.from(rowKey).toString('base64'),
familyName: {value: 'family'},
qualifier: {value: 'qualifier'},
valueSize: 0,
timestampMicros: 0,
labels: [],
qualifier: {value: Buffer.from('qualifier').toString('base64')},
commitRow: true,
value: 'value',
value: Buffer.from(rowKey).toString('base64'),
};
}

function getRequestOptions(request: any): google.bigtable.v2.IRowSet {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the old test, this contains all the code which was used for building a request.

const requestOptions = {} as google.bigtable.v2.IRowSet;
if (request.rows && request.rows.rowRanges) {
requestOptions.rowRanges = request.rows.rowRanges.map(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(range: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another type option than any that can be used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. I made this more specific and solved the resulting compiler errors too.

const convertedRowRange = {} as {[index: string]: string};
{
// startKey and endKey get filled in during the grpc request.
// They should be removed as the test data does not look
// for these properties in the request.
if (range.startKey) {
delete range.startKey;
}
if (range.endKey) {
delete range.endKey;
}
}
Object.keys(range).forEach(
key => (convertedRowRange[key] = range[key].asciiSlice())
);
return convertedRowRange;
}
);
}
if (request.rows && request.rows.rowKeys) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another type we can use besides any?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got rid of this comment and made it compile. This was copy/paste from the original test along with some other any's. Now the types are more specific, but a lot of interfaces and guards needed to be added to solve compiler errors up and down the stack.

requestOptions.rowKeys = request.rows.rowKeys.map((rowKeys: any) =>
rowKeys.asciiSlice()
);
}
// The grpc protocol sets rowsLimit to '0' if rowsLimit is not provided in the
// grpc request.
//
// Do not append rowsLimit to collection of request options if received grpc
// rows limit is '0' so that test data in read-rows-retry-test.json remains
// shorter.
if (request.rowsLimit && request.rowsLimit !== '0') {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(requestOptions as any).rowsLimit = parseInt(request.rowsLimit);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once again asking about the any 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one was a good one to fix. Helped catch some incorrect types downstream.

}
return requestOptions;
}

describe('Bigtable/Table', () => {
const bigtable = new Bigtable();
const INSTANCE_NAME = 'fake-instance2';
Expand Down Expand Up @@ -123,97 +130,89 @@ describe('Bigtable/Table', () => {
});
});

describe('createReadStream', () => {
let clock: sinon.SinonFakeTimers;
let endCalled: boolean;
let error: ServiceError | null;
let requestedOptions: Array<{}>;
let responses: Array<{}> | null;
let rowKeysRead: Array<Array<{}>>;
let stub: sinon.SinonStub;

beforeEach(() => {
clock = sinon.useFakeTimers({
toFake: [
'setTimeout',
'clearTimeout',
'setImmediate',
'clearImmediate',
'setInterval',
'clearInterval',
'Date',
'nextTick',
],
describe('createReadStream using mock server', () => {
let server: MockServer;
let service: MockService;
let bigtable = new Bigtable();
let table: Table;
before(async () => {
// make sure we have everything initialized before starting tests
const port = await new Promise<string>(resolve => {
server = new MockServer(resolve);
});
endCalled = false;
error = null;
responses = null;
rowKeysRead = [];
requestedOptions = [];
stub = sinon.stub(bigtable, 'request').callsFake(cfg => {
const reqOpts = cfg.reqOpts;
const requestOptions = {} as google.bigtable.v2.IRowSet;
if (reqOpts.rows && reqOpts.rows.rowRanges) {
requestOptions.rowRanges = reqOpts.rows.rowRanges.map(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(range: any) => {
const convertedRowRange = {} as {[index: string]: string};
Object.keys(range).forEach(
key => (convertedRowRange[key] = range[key].asciiSlice())
);
return convertedRowRange;
}
);
}
if (reqOpts.rows && reqOpts.rows.rowKeys) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
requestOptions.rowKeys = reqOpts.rows.rowKeys.map((rowKeys: any) =>
rowKeys.asciiSlice()
);
}
if (reqOpts.rowsLimit) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(requestOptions as any).rowsLimit = reqOpts.rowsLimit;
}
requestedOptions.push(requestOptions);
rowKeysRead.push([]);
const requestStream = new PassThrough({objectMode: true});
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(requestStream as any).abort = () => {};
dispatch(requestStream, responses!.shift());
return requestStream;
bigtable = new Bigtable({
apiEndpoint: `localhost:${port}`,
});
});

afterEach(() => {
clock.restore();
stub.restore();
table = bigtable.instance('fake-instance').table('fake-table');
service = new BigtableClientMockService(server);
});

tests.forEach(test => {
it(test.name, () => {
responses = test.responses;
TABLE.maxRetries = test.max_retries;
TABLE.createReadStream(test.createReadStream_options)
.on('data', row => rowKeysRead[rowKeysRead.length - 1].push(row.id))
.on('end', () => (endCalled = true))
.on('error', err => (error = err as ServiceError));
clock.runAll();

if (test.error) {
assert(!endCalled, ".on('end') should not have been invoked");
assert.strictEqual(error!.code, test.error);
} else {
assert(endCalled, ".on('end') shoud have been invoked");
assert.ifError(error);
it(test.name, done => {
// These variables store request/response data capturing data sent
// and received when using readRows with retries. This data is evaluated
// in checkResults at the end of the test for correctness.
const requestedOptions: google.bigtable.v2.IRowSet[] = [];
const responses = test.responses;
const rowKeysRead: any[] = [];
let endCalled = false;
let error: ServiceError | null = null;
function checkResults() {
if (test.error) {
assert(!endCalled, ".on('end') should not have been invoked");
assert.strictEqual(error!.code, test.error);
} else {
assert(endCalled, ".on('end') should have been invoked");
assert.ifError(error);
}
assert.deepStrictEqual(rowKeysRead, test.row_keys_read);
assert.strictEqual(
responses.length,
0,
'not all the responses were used'
);
assert.deepStrictEqual(requestedOptions, test.request_options);
done();
}
assert.deepStrictEqual(rowKeysRead, test.row_keys_read);
assert.strictEqual(
responses.length,
0,
'not all the responses were used'
);
assert.deepStrictEqual(requestedOptions, test.request_options);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These asserts are moved into checkResults which evaluates response/request data when the call to createreadstream results in an error or ended stream.


table.maxRetries = test.max_retries;
service.setService({
ReadRows: (
stream: ServerWritableStream<
protos.google.bigtable.v2.IReadRowsRequest,
protos.google.bigtable.v2.IReadRowsResponse
>
) => {
const response = responses!.shift();
assert(response);
rowKeysRead.push([]);
requestedOptions.push(getRequestOptions(stream.request));
if (response.row_keys) {
stream.write({
chunks: response.row_keys.map(rowResponseFromServer),
});
}
if (response.end_with_error) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const error: any = new Error();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an error coming from the server? If so is it a GoogleError not an any?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Changed to GoogleError.

error.code = response.end_with_error;
stream.emit('error', error);
} else {
stream.end();
}
},
});
table
.createReadStream(test.createReadStream_options)
.on('data', row => rowKeysRead[rowKeysRead.length - 1].push(row.id))
.on('end', () => {
endCalled = true;
checkResults();
})
.on('error', err => {
error = err as ServiceError;
checkResults();
});
});
});
});
Expand Down
21 changes: 21 additions & 0 deletions system-test/testTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import {ServiceError} from 'google-gax';
import {GetRowsOptions} from '../src/table';
import {google} from '../protos/protos';

export interface Test {
name: string;
Expand Down Expand Up @@ -46,3 +47,23 @@ export interface Test {
row_keys_read: {};
createReadStream_options: GetRowsOptions;
}

interface CreateReadStreamResponse {
row_keys?: string[];
end_with_error?: 4;
}

interface CreateReadStreamRequest {
rowKeys: string[];
rowRanges: google.bigtable.v2.IRowRange[];
rowsLimit?: number;
}
export interface ReadRowsTest {
createReadStream_options?: GetRowsOptions;
max_retries: number;
responses: CreateReadStreamResponse[];
request_options: CreateReadStreamRequest[];
error: number;
row_keys_read: string[][];
name: string;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In here there is an interesting mix of snake_case and camelCase - is there a reason why both have to be used? Or can they all be migrated to a more typical JS-y camelCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can be migrated, but it means that the json file containing all the test data in

"createReadStream_options": {
needs to be updated to match.

Loading