Getting latency results #1204
Closed
Getting latency results #1204
Conversation
This file contains 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
The fact that using a transaction object to do a commit results in a non-transaction should be documented so that if we decide to introduce a change later where this behaves differently then it is well documented. # Conflicts: # test/transaction.ts
When begin transaction sends back an error, we want some tests to capture what the behavior is so that when we make changes to the run function then behavior is preserved.
A response should reach the user the right way. Add tests to make sure behavior is preserved.
In the run function delegate calls to runAsync and use run async to make promise calls
This allows this function to return a promise instead of a promise wrapped in a promise. This makes the tests pass and behave the way they should.
Do not call request from self
Comments should actually explain what is being done
The commit test for this PR should be removed because it is not really relevant for the async run functionality.
The types used should be very specific so that reading the code isn’t confusing.
Add a type to the resolve function just to introduce more clarity
Make the types more specific in the data client callback so that it is easier to track down the signature and match against the begin transaction function.
The parsing logic is going to be needed elsewhere so taking it apart now.
The interface name should be changed so that it matches what it is. It is the callback used to form a promise.
The internals of commit should be moved to a new function. This way we can sandwich a commit async call between commit and runCommitAsync.
Still an issue with system tests. commit still needs tweaks to work with async.
Remove the no-op, get commit tests in place.
Mocks and additional debugging hooks to introspect what is going on.
Make sure commit behaves the same way as before.
Commit tests added to ensure that commit behaves the same way as before.
The tests should pass before we make changes to commit.
One of the mocks does not need to be written twice
…t-tests' of https://github.com/danieljbruce/nodejs-datastore into nodejs-transaction-redesign-feature-branch-2-adding-commit
…t-tests' of https://github.com/danieljbruce/nodejs-datastore into nodejs-transaction-redesign-feature-branch-3-introducing-async-commit # Conflicts: # test/transaction.ts
Make the promise simpler. Change the tests to exclude functions with promisify.
The TODO statements no longer need to be followed up on so remove them.
Remove the type definition. It is not used.
…into nodejs-transaction-redesign-feature-branch-2aa-adding-the-tests
…c functions and other read/write calls (#1196) * Add a unit test for commit as a non-tx The fact that using a transaction object to do a commit results in a non-transaction should be documented so that if we decide to introduce a change later where this behaves differently then it is well documented. # Conflicts: # test/transaction.ts * use linter * Write tests to capture current behavior of error When begin transaction sends back an error, we want some tests to capture what the behavior is so that when we make changes to the run function then behavior is preserved. * Add tests for passing a response A response should reach the user the right way. Add tests to make sure behavior is preserved. * run async close to working In the run function delegate calls to runAsync and use run async to make promise calls * Add runAsync to promise excludes This allows this function to return a promise instead of a promise wrapped in a promise. This makes the tests pass and behave the way they should. * Remove space * Change to use this instead of self Do not call request from self * Eliminate unused comments * Add two comments Comments should actually explain what is being done * Remove the commit test for this PR The commit test for this PR should be removed because it is not really relevant for the async run functionality. * Clarify types throughout the function The types used should be very specific so that reading the code isn’t confusing. * Add a bit more typing for clarity Add a type to the resolve function just to introduce more clarity * Change types used in the data client callback Make the types more specific in the data client callback so that it is easier to track down the signature and match against the begin transaction function. * run the linter * Add comments to clarify PR * Refactor the parsing logic out of run The parsing logic is going to be needed elsewhere so taking it apart now. * Change interface of request promise callback The interface name should be changed so that it matches what it is. It is the callback used to form a promise. * Hide data completely Change accessors to hide data completely instead of using the private modifier * PR use if/else block Eliminate the early return as suggested in the PR * Add comments to document the new functions The comments capture the parameters and return type. * Update return type in docs * Update the tests to include runAsync runAsync should be in promisfy excludes * refactor: Break transaction.run into smaller pieces for use with async functions and other read/write calls * Rename a function to be more descriptive Make sure it is explicit that we are parsing begin results. * Modify comment Modify comment so that it doesn’t reference the way the code was before.
…github.com/danieljbruce/nodejs-datastore into nodejs-transaction-redesign-feature-branch-2aa-adding-the-tests # Conflicts: # src/transaction.ts # test/transaction.ts
Refactor test code to provide better error catching and also not repeat setup code for the run function.
The comment for the runCommit method just captures the fact that the function is used as a pass-through.
The comments should more directly address the problem the tests are meant to solve
This describe block is now duplicated. Remove it.
This was a typo
…n function changes
…he-tests' of https://github.com/danieljbruce/nodejs-datastore into nodejs-transaction-redesign-feature-branch-2aa-adding-the-tests
…he-tests' of https://github.com/danieljbruce/nodejs-datastore into nodejs-transaction-redesign-feature-branch-5-adding-the-tests # Conflicts: # src/transaction.ts # test/transaction.ts
Another some more integration tests for runQuery, put, commit
…n’t run before some new tests we are going to add.
Add one of the test cases from the document (runAggregationQuery tests)
put, runAggregationQuery, commit tests have been added and now run properly.
The latency tests measure time taken with and without using the run call.
The logs will output the time required for the latency tests.
product-auto-label
bot
added
size: xl
The two tests for put, commit should make sure that begin transaction is called.
To meet the needs we want for unit testing we must add some checker that lets us record the requests and verify that they are the right values.
Modify the order tester object to include more calls. Also add expected requests to the existing tests.
Create test for a read, read and then commit.
Unit test should capture the fact that the run callback is used.
When using get requests without passing consistency, we should see two lookup requests reach the gapic layer. Consistency should be removed from the test because it is not meant to reach the Gapic layer.
console logs are not needed. Remove them so that the tests are cleaner.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Latency results