test: Eliminate repeated if statements in the ReadRows mock service reducing the size of it significantly #1460

danieljbruce · 2024-07-29T17:41:57Z

Summary:

This PR reduces all of those custom if statements down to one line of code in the ReadRows mock implementation cutting the ReadRows mock implementation down to about half the size so that in the next PR it is easy to make a modular version of the ReadRows mock implementation.

Background:

As requested, the eventual goal is to address these TODOs. So far, two PRs have already been merged to take steps toward this:

this PR eliminates duplicate code and adds interfaces to simplify data flows
this PR eliminated the duplicate copy of the ReadRows service

Changes:

Instead of having a global debug log, define a debugLog and pass the debugLog into the readrows service when creating the readrows service. This involves adding a debug log to the ReadRowsServiceParameters.
Add a DebugLog interface as it is now needed to specify the type for the debugLog dependency passed in to various functions now that the global debugLog has been eliminated.
Add an interface ReadRowsWritableStream for the ServerWritableStream<x,x> type used everywhere for conciseness. Then replace all references to ServerWritableStream<x,x> with the ReadRowsWritableStream type.
In readRowsImpl.ts replace all those repeated if statements with a call to generateChunksFromRequest. generateChunksFromRequest then calls a new generateChunks function with calculated keyTo and keyFrom values. keyTo and keyFrom are both calculated using a new getSelectedKey method which uses a default key if it is provided and otherwise uses the key provided in the open/close properties of the request.
Every time a ReadRows service is defined a debugLog is passed in as now this parameter is required.

Next Steps:

The readRowsImpl function will be broken down into classes as required by the TODO.

Only ignore keyFrom and keyTo when they are undefined and not just when they are Falsy.

The old service has been generalized enough to mock correct server behaviour.

they are undefined anyway.

…/github.com/googleapis/nodejs-bigtable into 3527322442-refactor-the-test-into-classes-2

…dejs-bigtable into 3527322442-refactor-the-test-into-classes-2

merge

module

danieljbruce · 2024-07-29T20:29:20Z

test/readrows.ts

+    console.log(text);
+  }
+}
+


DebugLog is just being moved from the service to here in the test

danieljbruce · 2024-07-29T20:30:18Z

test/utils/readRowsImpl.ts

-  if (DEBUG) {
-    console.log(text);
-  }
-}


This has just been moved to the test and will be passed into the service.

danieljbruce · 2024-07-29T20:43:02Z

test/utils/readRowsImpl.ts

-      chunkSize: serviceParameters.chunkSize,
-      valueSize: serviceParameters.valueSize,
-    });
+    const chunks = generateChunksFromRequest(stream.request, serviceParameters);


This single line of code is a substitute for all of the code deleted previous to it.

The code for each if statement now lives in getKeyValue. generateChunksFromRequest calls generateChunks making 2 calls to getSelectedKey each making 2 calls to getKeyValue.

* test: begin to refactor the mock service (#1447) * Use prettyPrintRequest from readRowsImpl1 * Define interfaces for creating a Bigtable service * Remove keyFrom and keyTo from each test * Define the service inline for standard keys errors * Add a comment about the refactor steps I am doing * Add a header to the service parameters file * Use the ChunkGeneratorParameters interface * Simplify the diff by creating local variables * Remove comment * Eliminate chunks per response constant * Change imports * Replace with as ServerImplementationInterface * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Add a second test for an error at a random pos * Add some comments for documentation * Chunk generation - add the parameter * Added comments to all the interfaces * Fix a regression bug from the merge --------- Co-authored-by: Leah E. Cole <[email protected]> * test: Eliminate the second ReadRows Service and use a generalized version of the first ReadRows (#1457) * Use prettyPrintRequest from readRowsImpl1 * Define interfaces for creating a Bigtable service * Remove keyFrom and keyTo from each test * Define the service inline for standard keys errors * Add a comment about the refactor steps I am doing * Add a header to the service parameters file * Use the ChunkGeneratorParameters interface * Simplify the diff by creating local variables * Remove comment * Eliminate chunks per response constant * Change imports * Set up the second ReadRows service to use params * Remove duplicate copy of generate chunks from serv * Remove second copy of isKeyInRowSet * Eliminate duplicate copies of the debug log * Fix a bug for the to and from service parameters Only ignore keyFrom and keyTo when they are undefined and not just when they are Falsy. * Add cancel back into the mock service for testing * Add variables to match service 1 * Add one more check to match the other service * Remove usages of ReadRows2Impl * Remove the new service The old service has been generalized enough to mock correct server behaviour. * Moved the position of the comment for 150 rows * Eliminate setting keyTo and keyFrom they are undefined anyway. * Add a second test for an error at a random pos * Add some comments for documentation * Chunk generation - add the parameter * Added comments to all the interfaces * Delete file * Change splitted to split * Remove export * Don’t rename the interfaces There is no point * Increase Windows timeout * Provide functions to eliminate if statements * Eliminate the if statements Make a direct call to generateChunksFromRequest * Revert "Eliminate the if statements" This reverts commit 0996e89. * Revert "Provide functions to eliminate if statements" This reverts commit 4a4761f. * Change any’s to string | undefined * Eliminate duplicate code for setting timeouts * remove only * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> --------- Co-authored-by: Leah E. Cole <[email protected]> * test: Eliminate repeated if statements in the ReadRows mock service reducing the size of it significantly (#1460) * Use prettyPrintRequest from readRowsImpl1 * Define interfaces for creating a Bigtable service * Remove keyFrom and keyTo from each test * Define the service inline for standard keys errors * Add a comment about the refactor steps I am doing * Add a header to the service parameters file * Use the ChunkGeneratorParameters interface * Simplify the diff by creating local variables * Remove comment * Eliminate chunks per response constant * Change imports * Set up the second ReadRows service to use params * Remove duplicate copy of generate chunks from serv * Remove second copy of isKeyInRowSet * Eliminate duplicate copies of the debug log * Fix a bug for the to and from service parameters Only ignore keyFrom and keyTo when they are undefined and not just when they are Falsy. * Add cancel back into the mock service for testing * Add variables to match service 1 * Add one more check to match the other service * Remove usages of ReadRows2Impl * Remove the new service The old service has been generalized enough to mock correct server behaviour. * Moved the position of the comment for 150 rows * Eliminate setting keyTo and keyFrom they are undefined anyway. * Add a second test for an error at a random pos * Add some comments for documentation * Chunk generation - add the parameter * Added comments to all the interfaces * Group the property getter into a function * Group key selection into functions * Fix typo: default key * Documentation for isInRowSet * Eliminate variable - move function inline * Omit optional selector on stream * Create ReadRowsWritableStream interface * Use new interface, remove ServerWritableStream * Don’t pass the whole stream into helpers * Add a function for generating the chunks * The debug log should be a pass through parameter * Solve compiler errors resulting immediately from merge * Add debugLog parameter to various functions * Add return type * Remove exports - functions are only used in this module * Revise merge correction * Remove TODO for the stream type * Update the getKeyValue function * Eliminate place where debug log is needed * Run linter * test: Break the ReadRows service down into classes instead of a single long function (#1461) * Use prettyPrintRequest from readRowsImpl1 * Define interfaces for creating a Bigtable service * Remove keyFrom and keyTo from each test * Define the service inline for standard keys errors * Add a comment about the refactor steps I am doing * Add a header to the service parameters file * Use the ChunkGeneratorParameters interface * Simplify the diff by creating local variables * Remove comment * Eliminate chunks per response constant * Change imports * Set up the second ReadRows service to use params * Remove duplicate copy of generate chunks from serv * Remove second copy of isKeyInRowSet * Eliminate duplicate copies of the debug log * Fix a bug for the to and from service parameters Only ignore keyFrom and keyTo when they are undefined and not just when they are Falsy. * Add cancel back into the mock service for testing * Add variables to match service 1 * Add one more check to match the other service * Remove usages of ReadRows2Impl * Remove the new service The old service has been generalized enough to mock correct server behaviour. * Moved the position of the comment for 150 rows * Eliminate setting keyTo and keyFrom they are undefined anyway. * Add a second test for an error at a random pos * Add some comments for documentation * Chunk generation - add the parameter * Added comments to all the interfaces * Group the property getter into a function * Group key selection into functions * Fix typo: default key * Documentation for isInRowSet * Eliminate variable - move function inline * Omit optional selector on stream * Create ReadRowsWritableStream interface * Use new interface, remove ServerWritableStream * Don’t pass the whole stream into helpers * Add a function for generating the chunks * The debug log should be a pass through parameter * Add some TODOs about how to address this next * Pull send response into a class * Create a dedicated class for defining the service * Change name to readRowsRequestHandler * Pull the function that generates the chunks into Separate module * Generate documentation * Add more documentation to the Service class * Add debugLog as a method parameter * Solve compiler errors resulting immediately from merge * Add debugLog parameter to various functions * Add return type * Remove exports - functions are only used in this module * Revise merge correction * Remove TODO for the stream type * Update the getKeyValue function * Eliminate place where debug log is needed * Run linter * Eliminate the function that creates a service Add a factory method instead * Add documentation for the constructor * Remove the TODO * Set the timeout for the test * Increase the timeout * Eliminate ternary statement * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * Add a couple of comments * re-write the ternary expression --------- Co-authored-by: Leah E. Cole <[email protected]> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

danieljbruce added 30 commits July 16, 2024 14:14

Use prettyPrintRequest from readRowsImpl1

56aa444

Define interfaces for creating a Bigtable service

0699bf7

Remove keyFrom and keyTo from each test

f77261a

Define the service inline for standard keys errors

958a255

Add a comment about the refactor steps I am doing

5558d00

Add a header to the service parameters file

7589b60

Use the ChunkGeneratorParameters interface

ec545e9

Simplify the diff by creating local variables

15479c9

Remove comment

588234a

Eliminate chunks per response constant

de7d357

Change imports

228afb1

Set up the second ReadRows service to use params

786fb66

Remove duplicate copy of generate chunks from serv

95fd859

Remove second copy of isKeyInRowSet

e2837a5

Eliminate duplicate copies of the debug log

c45b0bb

Fix a bug for the to and from service parameters

af2955b

Only ignore keyFrom and keyTo when they are undefined and not just when they are Falsy.

Add cancel back into the mock service for testing

68e9d04

Add variables to match service 1

a7e13df

Add one more check to match the other service

5bd8a55

Remove usages of ReadRows2Impl

e16b3c9

Remove the new service

952a6a9

The old service has been generalized enough to mock correct server behaviour.

Moved the position of the comment for 150 rows

38b0006

Eliminate setting keyTo and keyFrom

716008a

they are undefined anyway.

Add a second test for an error at a random pos

22848fe

Add some comments for documentation

917f3dc

Chunk generation - add the parameter

799f5ef

Added comments to all the interfaces

6be2743

Merge branch '3527322442-refactor-the-test-into-classes-2' of https:/…

8f9ffa0

…/github.com/googleapis/nodejs-bigtable into 3527322442-refactor-the-test-into-classes-2

Merge branch '3527322442-to-main' of https://github.com/googleapis/no…

ab81643

…dejs-bigtable into 3527322442-refactor-the-test-into-classes-2

Group the property getter into a function

1b0dec8

Solve compiler errors resulting immediately from

7162866

merge

product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Jul 29, 2024

danieljbruce added 3 commits July 29, 2024 13:48

Add debugLog parameter to various functions

882913b

Add return type

cefe405

Remove exports - functions are only used in this

8681d45

module

danieljbruce changed the title ~~test: TODO~~ test: Eliminate repeated if statements in the ReadRows mock service reducing the size of it significantly Jul 29, 2024

Revise merge correction

5ea0cc0

danieljbruce commented Jul 29, 2024

View reviewed changes

danieljbruce added 3 commits July 29, 2024 16:31

Remove TODO for the stream type

f771841

Update the getKeyValue function

25ca84f

Eliminate place where debug log is needed

b907a31

danieljbruce commented Jul 29, 2024

View reviewed changes

Run linter

ae3447c

danieljbruce marked this pull request as ready for review July 29, 2024 20:52

danieljbruce requested review from a team as code owners July 29, 2024 20:52

leahecole requested review from danielbankhead and leahecole July 30, 2024 14:23

bhshkh approved these changes Aug 9, 2024

View reviewed changes

danieljbruce mentioned this pull request Aug 12, 2024

test: Break the ReadRows service down into classes instead of a single long function #1461

Merged

leahecole approved these changes Aug 12, 2024

View reviewed changes

danielbankhead approved these changes Aug 12, 2024

View reviewed changes

danieljbruce removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 12, 2024

danieljbruce merged commit fa70453 into 3527322442-to-main-2 Aug 12, 2024

danieljbruce deleted the 3527322442-refactor-the-test-into-classes-3 branch August 12, 2024 20:11

danieljbruce mentioned this pull request Aug 19, 2024

test: Make the ReadRows service in tests more modular #1462

Merged

kevkim-codes mentioned this pull request Sep 27, 2024

test: Make the ReadRows service in tests more modular (#1462) kevkim-codes/nodejs-bigtable#1

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: Eliminate repeated if statements in the ReadRows mock service reducing the size of it significantly #1460

test: Eliminate repeated if statements in the ReadRows mock service reducing the size of it significantly #1460

Uh oh!

danieljbruce commented Jul 29, 2024 •

edited

Loading

Uh oh!

danieljbruce Jul 29, 2024 •

edited

Loading

Uh oh!

danieljbruce Jul 29, 2024

Uh oh!

danieljbruce Jul 29, 2024

Uh oh!

danieljbruce Jul 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

test: Eliminate repeated if statements in the ReadRows mock service reducing the size of it significantly #1460

test: Eliminate repeated if statements in the ReadRows mock service reducing the size of it significantly #1460

Uh oh!

Conversation

danieljbruce commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danieljbruce Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieljbruce Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

danieljbruce Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

danieljbruce Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danieljbruce commented Jul 29, 2024 •

edited

Loading

danieljbruce Jul 29, 2024 •

edited

Loading