test: Eliminate the second ReadRows Service and use a generalized version of the first ReadRows#1457
Merged
danieljbruce merged 49 commits into3527322442-to-main-2from Jul 29, 2024
Conversation
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
sofisl
requested changes
Jul 24, 2024
test/utils/readRowsImpl.ts
Outdated
|
|
||
| let chunksSent = 0; | ||
| const chunks = generateChunks(serviceParameters); | ||
| let keyFromRequestClosed: any; |
Contributor
Author
There was a problem hiding this comment.
I made changes so that the entire file doesn't contain any type. This entire repeated block of if statements has already been replaced with one function call in the next branch/PR anyways.
sofisl
approved these changes
Jul 26, 2024
leahecole
approved these changes
Jul 29, 2024
test/readrows.ts
Outdated
Comment on lines
+342
to
+344
| if (process.platform === 'win32') { | ||
| this.timeout(60000); // it runs much slower on Windows! | ||
| } |
Contributor
There was a problem hiding this comment.
nit - could this be in a beforeeach?
Contributor
Author
There was a problem hiding this comment.
I grouped this repeated code into a setTestTimeout function. Unfortunately, we can't use beforeEach because you will notice the code uses this which is a reference to the test context. If we put this code into a beforeEach hook then this.timeout would set the timeout on the beforeEach and not on the it.
leahecole
requested changes
Jul 29, 2024
Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
leahecole
approved these changes
Jul 29, 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:
This PR is a follow-up to #1447. It eliminates the second ReadRows service entirely.
Background:
To solve a high priority issue previously I created
readRowsImpl2.tsby copy/pasting code fromreadRowsImpl.tsand making tweaks. There wasn't enough time to investigate ifreadRowsImpl.tscould be generalized to do whatreadRowsImpl2.tswas doing for us so we are addressing that technical debt now.Changes:
readRowsImpl2.tsis deleted entirely.readRowsImpl.tshas been generalized so that whenkeyFromandkeyToare not provided then the keys from the request parameters will be used. This was howreadRowsImpl2.tsworked.readRowsImpl2will now use the generalized version ofreadRowsImpland when not providing thekeyFromandkeyToparameters then the key range in the generated chunks will be determined by the values passed into the request.Next steps: