test: begin to refactor the mock service#1447
Merged
danieljbruce merged 21 commits into3527322442-to-main-2from Jul 23, 2024
Merged
test: begin to refactor the mock service#1447danieljbruce merged 21 commits into3527322442-to-main-2from
danieljbruce merged 21 commits into3527322442-to-main-2from
Conversation
leahecole
requested changes
Jul 17, 2024
test/readrows.ts
Outdated
|
|
||
| service.setService({ | ||
| ReadRows: readRowsImpl(keyFrom, keyTo) as any, | ||
| ReadRows: readRowsImpl(STANDARD_SERVICE_WITHOUT_ERRORS) as any, |
Contributor
There was a problem hiding this comment.
I would love it if we could have no typecasting to any in this PR
Contributor
Author
There was a problem hiding this comment.
I just replaced all these instances with as ServerImplementationInterface.
test/readrows.ts
Outdated
| valueSize: VALUE_SIZE, | ||
| chunkSize: CHUNK_SIZE, | ||
| chunksPerResponse: CHUNKS_PER_RESPONSE, | ||
| errorAfterChunkNo: 423, |
Contributor
There was a problem hiding this comment.
any reason why we're using 423 or is it just an arbitrary spot to error?
Contributor
Author
There was a problem hiding this comment.
This was just the value that Alex used for this test. It looks like it is just a value that is as random as possible to make the test realistic.
Contributor
There was a problem hiding this comment.
Curious if we could use Math.random() to test it more realistically.
Contributor
Author
There was a problem hiding this comment.
I added an additional test to emit an error at a random position. I want to keep the old test in place too so that if it breaks then we have predictable code we can use to debug the issue.
| import {ServerWritableStream} from '@grpc/grpc-js'; | ||
| import {protos} from '../../src'; | ||
| import {GoogleError, Status} from 'google-gax'; | ||
| import {prettyPrintRequest} from './readRowsImpl'; |
Contributor
There was a problem hiding this comment.
(feel free to resolve) YAY FOR REUSING THINGS 😁
Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
sofisl
reviewed
Jul 17, 2024
| import {ReadRowsServiceParameters} from '../test/utils/readRowsServiceParameters'; | ||
|
|
||
| // Define parameters for a standard Bigtable Mock service | ||
| const VALUE_SIZE = 1024 * 1024; |
Contributor
There was a problem hiding this comment.
what does value size represent?
Contributor
Author
There was a problem hiding this comment.
I added a comment for this. It is // An upper bound on the amount of data included in the chunks
…ithub.com/googleapis/nodejs-bigtable into 3527322442-refactor-the-test-into-classes
…into 3527322442-refactor-the-test-into-classes
…dejs-bigtable into 3527322442-refactor-the-test-into-classes
sofisl
approved these changes
Jul 23, 2024
This was referenced Aug 12, 2024
danieljbruce
added a commit
that referenced
this pull request
Sep 23, 2024
* 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 <6719667+leahecole@users.noreply.github.com> * 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 <6719667+leahecole@users.noreply.github.com> * 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 <6719667+leahecole@users.noreply.github.com> * Update test/readrows.ts Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com> * Update test/readrows.ts Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com> * Update test/readrows.ts Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com> * Update test/readrows.ts Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com> * Update test/readrows.ts Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com> * Update test/readrows.ts Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com> --------- Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com> * 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 <6719667+leahecole@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
The ultimate goal is to address the TODOs mentioned in
nodejs-bigtable/test/utils/readRowsImpl2.ts
Lines 161 to 163 in 34d138d
We are going to stack PRs onto this branch and then make one final merge into main.
Plan:
Currently there are 2 ReadRows mock services with a bunch of code that is duplicated. Before we address the excessive amount of if statements we need to take steps to eliminate similar services
ReadRowsImpl1andReadRowsImpl2. And before we eliminate duplicate code inReadRowsImplandReadRowsImpl2we should take some basic steps toward trying to eliminateReadRowsImpl2entirely by allowing for additional configurations to be made toReadRowsImplso that it can be reused.In this PR:
ReadRowsImpl,ReadRowsImpl2each rely on a different set of constants for chunk size, value size and chunks per response. Allowing them to be configurable inReadRowsImplgeneralizesReadRowsImpland is one step toward our plan of replacingReadRowsImpl2with a generalized version ofReadRowsImpl.ReadRowsServiceParametersandChunkGeneratorParametersso thatreadRowsImplandgenerateChunksonly need one parameter passed in.const keyFrom = 0;andconst keyTo = 1000;many times and we can just rely on a constant for these defined at the top of the file.ReadRowsImpl2by removingprettyPrintRequestbecause it is an exact duplicate of theprettyPrintRequestfunction inReadRowsImpl.Next steps:
ReadRowsImpl2.ReadRowsImpl2with a generalized version ofReadRowsImpl.ifstatements inReadRowsImpl. Introduce classes, generate unit tests, make the service more modular.