test: rewrite createreadstream tests #1401

danieljbruce · 2024-04-12T19:39:39Z

When retries are moved to google-gax from the handwritten layer of nodejs-bigtable then the current tests mentioned in the PR will not work. They need to be rewritten to still test retry behavior, but account for the fact that the retries will be done in a different layer.

Here is what is done to modify Tests in [read-rows.ts] just to clarify further:

This sinon stub mock currently mocks out the request function and sends responses back to createreadstream where retries are currently done. Since retries will not be done in createreadstream anymore and this test intends to test retries then responses must be mocked in a layer below where retries are going to be done in google-gax in a later change. The current sinon stub mock should be removed and instead the mock grpc server should send responses back in order to test retry behavior.
As is done with the current mock, the new mock server mock should check each request to ensure it matches request_options

- Transform the rowsLimit parameter into an integer - Change the hook into a before hook so that we don’t attempt to create multiple mock servers - Create a guard so that the stream only writes if there are row keys to write

define new interfaces too

danieljbruce

Added a few explanations throughout the PR

danieljbruce · 2024-04-12T19:49:22Z

src/util/mock-servers/mock-server.ts

@@ -37,7 +37,6 @@ export class MockServer {
      `localhost:${this.port}`,
      grpc.ServerCredentials.createInsecure(),
      () => {
-        server.start();


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

danieljbruce · 2024-04-12T19:54:25Z

system-test/read-rows.ts

 const {tests} = require('../../system-test/data/read-rows-retry-test.json') as {
-  tests: Test[];
+  tests: ReadRowsTest[];


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.

danieljbruce · 2024-04-12T19:59:12Z

system-test/read-rows.ts

-  }
-}
-
-function rowResponse(rowKey: {}) {


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.

danieljbruce · 2024-04-12T20:01:45Z

system-test/read-rows.ts

  };
 }

+function getRequestOptions(request: any): google.bigtable.v2.IRowSet {


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

danieljbruce · 2024-04-12T20:05:14Z

system-test/read-rows.ts

-          0,
-          'not all the responses were used'
-        );
-        assert.deepStrictEqual(requestedOptions, test.request_options);


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

leahecole · 2024-04-16T20:10:47Z

system-test/read-rows.ts

+  if (request.rows && request.rows.rowRanges) {
+    requestOptions.rowRanges = request.rows.rowRanges.map(
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      (range: any) => {


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

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

leahecole · 2024-04-16T20:11:38Z

system-test/read-rows.ts

+    );
+  }
+  if (request.rows && request.rows.rowKeys) {
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any


Is there another type we can use besides any?

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.

leahecole · 2024-04-16T20:12:09Z

system-test/read-rows.ts

+  // shorter.
+  if (request.rowsLimit && request.rowsLimit !== '0') {
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    (requestOptions as any).rowsLimit = parseInt(request.rowsLimit);


once again asking about the any 🙂

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

leahecole · 2024-04-16T20:16:57Z

system-test/read-rows.ts

+            }
+            if (response.end_with_error) {
+              // eslint-disable-next-line @typescript-eslint/no-explicit-any
+              const error: any = new Error();


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

Yup. Changed to GoogleError.

leahecole · 2024-04-16T20:18:43Z

system-test/testTypes.ts

+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;
+}


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?

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

nodejs-bigtable/system-test/data/read-rows-retry-test.json

Line 112 in 3f5db96

"createReadStream_options": {

needs to be updated to match.

leahecole · 2024-04-16T20:24:18Z

Is the failure of the conformance tests expected?

danieljbruce · 2024-04-22T21:17:42Z

Is the failure of the conformance tests expected?

Yup. Right now the conformance tests are supposed to be running, but a whole bunch of them fail.

It was necessary to add guards to make this work. Also changed the error type to Google error.

danieljbruce added 7 commits April 11, 2024 11:55

set up the test frame

1b1947a

Test is set up to evaluate streaming behavior

59b2c26

Modify test with the mock server to pass first tst

baa5d8f

Fix tests that are appending startKey and endKey

869d5ce

Getting all the tests working

5e69834

- Transform the rowsLimit parameter into an integer - Change the hook into a before hook so that we don’t attempt to create multiple mock servers - Create a guard so that the stream only writes if there are row keys to write

eliminate old createreadstream test

bf29061

define new interfaces too

Remove only. rowsLimit should be optional

ad36097

product-auto-label bot added size: l Pull request size is large. api: bigtable Issues related to the googleapis/nodejs-bigtable API. labels Apr 12, 2024

danieljbruce marked this pull request as ready for review April 12, 2024 19:43

danieljbruce requested review from a team as code owners April 12, 2024 19:43

danieljbruce commented Apr 12, 2024

View reviewed changes

Make rowKeysRead type more specific

2c57e61

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 12, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 12, 2024

Add after hook to shut down server

95b9463

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 16, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 16, 2024

danieljbruce requested a review from leahecole April 16, 2024 14:07

danieljbruce changed the base branch from main to move-retries-to-google-gax April 16, 2024 20:09

leahecole requested changes Apr 16, 2024

View reviewed changes

danieljbruce added 6 commits April 23, 2024 15:41

First PR correction - remove any

23919c5

Get rid of another any

abcf0fc

Get rid of the any suppressed by typescript

07ec565

It was necessary to add guards to make this work. Also changed the error type to Google error.

Should use CreateReadStreamRequest as the type

85a0317

Remove the comment disabling linting on any

286e782

Remove the import. It is not used anymore.

f4136c1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: rewrite createreadstream tests #1401

test: rewrite createreadstream tests #1401

danieljbruce commented Apr 12, 2024 •

edited

danieljbruce left a comment

danieljbruce Apr 12, 2024 •

edited

danieljbruce Apr 12, 2024

danieljbruce Apr 12, 2024

danieljbruce Apr 12, 2024

danieljbruce Apr 12, 2024

leahecole Apr 16, 2024

danieljbruce Apr 23, 2024

leahecole Apr 16, 2024

danieljbruce Apr 23, 2024

leahecole Apr 16, 2024

danieljbruce Apr 23, 2024

leahecole Apr 16, 2024

danieljbruce Apr 23, 2024

leahecole Apr 16, 2024

danieljbruce Apr 23, 2024

leahecole commented Apr 16, 2024

danieljbruce commented Apr 22, 2024

test: rewrite createreadstream tests #1401

Are you sure you want to change the base?

test: rewrite createreadstream tests #1401

Conversation

danieljbruce commented Apr 12, 2024 • edited

danieljbruce left a comment

Choose a reason for hiding this comment

danieljbruce Apr 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leahecole commented Apr 16, 2024

danieljbruce commented Apr 22, 2024

danieljbruce commented Apr 12, 2024 •

edited

danieljbruce Apr 12, 2024 •

edited