tests: conformance test fixes 2 (#1770)

* tests: roll cumulative test fixes from https://github.com/googleapis/nodejs-bigtable/tree/conformance-fixes-2 (manually due to merging errors)

* chore: remove empty file

* tests: change 'closed' wording to match previously expected wording

* tests: also handle the stream mode being closed already

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* tests: remove extraneous bigtable.close() call

* tests: put back the other close line, and add a comment about why it looks duplicate

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

Author: 2 people

handwritten/bigtable/src/index.ts

@@ -487,6 +487,7 @@ export class Bigtable {
   static Cluster: Cluster;
   _metricsConfigManager: ClientSideMetricsConfigManager;
   admin: admin.BigtableAdmin;
+  closed = false;
 
   constructor(options: BigtableOptions = {}) {
     // Determine what scopes are needed.
@@ -904,6 +905,27 @@ export class Bigtable {
     let gaxStream: gax.CancellableStream;
     let stream: AbortableDuplex;
 
+    if (this.closed) {
+      const error = Object.assign(
+        new Error('The client has already been closed.'),
+        {
+          name: 'Closed',
+          code: grpc.status.ABORTED,
+          details: 'The client has already been closed.',
+          metadata: new grpc.Metadata(),
+        },
+      );
+      if (isStreamMode) {
+        stream = streamEvents(new PassThrough({objectMode: true}));
+        stream.abort = () => {};
+        setImmediate(() => stream.destroy(error));
+        return stream;
+      } else {
+        callback?.(error as ServiceError);
+        return;
+      }
+    }
+
     const prepareGaxRequest = (
       callback: (err: Error | null, fn?: Function) => void,
     ) => {
@@ -1021,11 +1043,22 @@ export class Bigtable {
    * Close all bigtable clients. New requests will be rejected but it will not
    * kill connections with pending requests.
    */
-  close(): Promise<void[]> {
+  async close(): Promise<void[]> {
+    // Close all of the clients.
     const combined = Object.keys(this.api).map(clientType =>
       this.api[clientType].close(),
     );
-    return Promise.all(combined);
+    const results = await Promise.all(combined);
+
+    // Clear them out of our cache.
+    Object.keys(this.api).forEach(clientType => {
+      delete this.api[clientType];
+    });
+
+    // Mark as closed.
+    this.closed = true;
+
+    return results;
   }
 
   /**

handwritten/bigtable/src/row.ts

@@ -30,7 +30,7 @@ import {CallOptions} from 'google-gax';
 import {ServiceError} from 'google-gax';
 import {google} from '../protos/protos';
 import {RowDataUtils, RowProperties} from './row-data-utils';
-import {TabularApiSurface} from './tabular-api-surface';
+import {GetRowsOptions, TabularApiSurface} from './tabular-api-surface';
 import {getRowsInternal} from './utils/getRowsInternal';
 import {
   MethodName,
@@ -667,9 +667,10 @@ export class Row {
       filter = arrify(filter).concat(options.filter);
     }
 
-    const getRowsOptions = Object.assign({}, options, {
+    const getRowsOptions: GetRowsOptions = Object.assign({}, options, {
       keys: [this.id],
       filter,
+      limit: 1,
     });
 
     const metricsCollector =

handwritten/bigtable/src/tabular-api-surface.ts

@@ -476,6 +476,11 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
       reqOpts,
       gaxOpts: Object.assign({}, gaxOptions),
     });
+    if (!requestStream) {
+      throw new Error(
+        'Failed to create request stream -- is the client already closed?',
+      );
+    }
     metricsCollector.wrapRequest(requestStream);
     const stream = pumpify.obj([requestStream, rowKeysStream]);
     stream.on('end', () => {

handwritten/bigtable/testproxy/known_failures.txt

@@ -1,20 +0,0 @@
-TestMutateRow_Generic_DeadlineExceeded\|
-TestMutateRow_Generic_CloseClient\|
-TestMutateRow_Generic_DeadlineExceeded\|
-TestReadModifyWriteRow_Generic_DeadlineExceeded\|
-TestReadRow_Generic_DeadlineExceeded\|
-TestMutateRows_Retry_WithRoutingCookie\|
-TestReadRow_Generic_DeadlineExceeded\|
-TestReadRow_Retry_WithRoutingCookie\|
-TestReadRow_Retry_WithRetryInfo\|
-TestReadRows_ReverseScans_FeatureFlag_Enabled\|
-TestReadRows_NoRetry_OutOfOrderError_Reverse\|
-TestReadRows_Retry_LastScannedRow_Reverse\|
-TestReadRows_Retry_WithRoutingCookie\|
-TestReadRows_Retry_WithRoutingCookie_MultipleErrorResponses\|
-TestReadRows_Retry_WithRetryInfo\|
-TestReadRows_Retry_WithRetryInfo_MultipleErrorResponse\|
-TestSampleRowKeys_Retry_WithRoutingCookie\|
-TestSampleRowKeys_Generic_CloseClient\|
-TestSampleRowKeys_Generic_Headers\|
-TestSampleRowKeys_NoRetry_NoEmptyKey\|

handwritten/bigtable/testproxy/services/close-client.ts

@@ -14,9 +14,12 @@
 
 import {google} from '../protos/protos';
 import {ClientImplMaker, normalizeCallback} from './utils';
+import {closeBigtableClient} from './utils/bigtable-client';
 type ICloseClientRequest = google.bigtable.testproxy.ICloseClientRequest;
 type ICloseClientResponse = google.bigtable.testproxy.ICloseClientResponse;
 
+import {log} from './utils/log';
+
 export const closeClient: ClientImplMaker<
   ICloseClientRequest,
   ICloseClientResponse
@@ -26,8 +29,18 @@ export const closeClient: ClientImplMaker<
     const {clientId} = request;
     const bigtable = clientMap.get(clientId!);
 
+    log.info(
+      'close client %s (%s)',
+      clientId,
+      bigtable ? 'exists' : "doesn't exist",
+    );
+
     if (bigtable) {
+      // closeBigtableClient closes the BigtableClient, but not the Bigtable
+      // object itself. We need to close the Bigtable object as well.
+      await closeBigtableClient(bigtable);
       await bigtable.close();
+      log.info('client %s closed', clientId);
     }
     return {};
   });

handwritten/bigtable/testproxy/services/create-client.ts

@@ -18,6 +18,7 @@ import {google} from '../protos/protos';
 import * as grpc from '@grpc/grpc-js';
 import {Bigtable} from '../../src';
 import {createBigtableClient} from './utils/bigtable-client';
+import {log} from './utils/log';
 
 function durationToMilliseconds(
   duration: google.protobuf.Duration | google.protobuf.IDuration,
@@ -68,6 +69,12 @@ export const createClient: ClientImplMaker<
       );
     }
 
+    log.info(
+      'create client %s (%s)',
+      clientId,
+      clientMap.has(clientId) ? 'exists' : "doesn't exist",
+    );
+
     if (clientMap.has(clientId)) {
       throw Object.assign(new Error(`Client ${clientId} already exists`), {
         code: grpc.status.ALREADY_EXISTS,
@@ -100,6 +107,7 @@ export const createClient: ClientImplMaker<
       appProfileId: appProfileId!,
       clientConfig,
     };
+    log.info('created bigtable client %s', clientId);
     const bigtable = new Bigtable(options);
     createBigtableClient(bigtable);
     clientMap.set(clientId!, bigtable);

handwritten/bigtable/testproxy/services/mutate-row.ts

@@ -34,14 +34,25 @@ export const mutateRow: ClientImplMaker<
     const appProfileId = bigtable.appProfileId;
     const client = getBigtableClient(bigtable);
 
-    await client.mutateRow({
-      appProfileId,
-      mutations,
-      tableName,
-      rowKey,
-    });
+    try {
+      await client.mutateRow({
+        appProfileId,
+        mutations,
+        tableName,
+        rowKey,
+      });
 
-    return {
-      status: {code: grpc.status.OK, details: []},
-    };
+      return {
+        status: {code: grpc.status.OK, details: []},
+      };
+    } catch (e) {
+      const error = e as GoogleError;
+      return {
+        status: {
+          code: error.code ? error.code : grpc.status.UNKNOWN,
+          message: error.message,
+          details: [],
+        },
+      };
+    }
   });

handwritten/bigtable/testproxy/services/read-row.ts

@@ -24,6 +24,7 @@ import {
   getRowResponse,
   getTableInfo,
 } from './utils';
+import {GoogleError} from 'google-gax';
 
 export const readRow: ClientImplMaker<IReadRowRequest, IRowResult> = ({
   clientMap,
@@ -32,15 +33,27 @@ export const readRow: ClientImplMaker<IReadRowRequest, IRowResult> = ({
     const {clientId, rowKey, tableName} = rawRequest.request;
     const columns = {};
 
-    const bigtable = clientMap.get(clientId!);
-    const table = getTableInfo(bigtable, tableName!);
-    const row = table.row(rowKey!);
+    try {
+      const bigtable = clientMap.get(clientId!);
+      const table = getTableInfo(bigtable, tableName!);
+      const row = table.row(rowKey!);
 
-    const res = await row.get(columns);
-    const firstRow = getRowResponse(res[0]);
+      const res = await row.get(columns);
+      const firstRow = getRowResponse(res[0]);
 
-    return {
-      status: {code: grpc.status.OK, details: []},
-      row: firstRow,
-    };
+      return {
+        status: {code: grpc.status.OK, details: []},
+        row: firstRow,
+      };
+    } catch (e) {
+      const error = e as GoogleError;
+      return {
+        status: {
+          code: error.code || grpc.status.FAILED_PRECONDITION,
+          // e.details must be in an empty array for the test runner to return the status. This is tracked in b/383096533.
+          details: [],
+          message: error.message,
+        },
+      };
+    }
   });

handwritten/bigtable/testproxy/services/read-rows.ts

@@ -87,7 +87,8 @@ export const readRows: ClientImplMaker<IReadRowsRequest, IRowsResult> = ({
       const error = e as GoogleError;
       return {
         status: {
-          code: error.code,
+          // This might be zero/undefined if it's a disconnected client error.
+          code: error.code || grpc.status.FAILED_PRECONDITION,
           // e.details must be in an empty array for the test runner to return the status. This is tracked in b/383096533.
           details: [],
           message: error.message,

handwritten/bigtable/testproxy/services/sample-row-keys.ts

@@ -14,10 +14,10 @@
 
 import * as grpc from '@grpc/grpc-js';
 import {GoogleError} from 'google-gax';
-import {getSRKRequest} from './utils/request/sampleRowKeys';
 import {ClientImplMaker, normalizeCallback} from './utils';
 
 import {google} from '../protos/protos';
+import {log} from './utils/log';
 type ISampleRowKeysRequest = google.bigtable.testproxy.ISampleRowKeysRequest;
 type ISampleRowKeysResult = google.bigtable.testproxy.ISampleRowKeysResult;
 
@@ -28,25 +28,32 @@ export const sampleRowKeys: ClientImplMaker<
   normalizeCallback(async rawRequest => {
     const {request} = rawRequest;
     const {clientId, request: sampleRowKeysRequest} = request;
-    const {appProfileId, tableName} = sampleRowKeysRequest!;
+    const {tableName} = sampleRowKeysRequest!;
 
     const bigtable = clientMap.get(clientId!);
-    bigtable.appProfileId = appProfileId!;
+
+    const [, , , instanceId, , tableId] = tableName!.split('/');
+    const instance = bigtable.instance(instanceId);
+    const table = instance.table(tableId);
 
     try {
-      const response = await getSRKRequest(bigtable, {appProfileId, tableName});
+      const response = await table.sampleRowKeys();
 
+      log.info('sampleRowKeys response %o', response);
       return {
         status: {code: grpc.status.OK, details: []},
-        response,
+        samples: response[0].map(sample => ({
+          rowKey: sample.key,
+          offsetBytes: sample.offset,
+        })),
       };
     } catch (e) {
       const error = e as GoogleError;
-      console.error('Error:', error.code);
 
       return {
         status: {
-          code: error.code,
+          // This might be zero/undefined if it's a disconnected client error.
+          code: error.code || grpc.status.FAILED_PRECONDITION,
           details: [],
         },
       };