Conversation
|
Warning: This pull request is touching the following templated files:
|
…Generic_DeadlineExceeded" (unit tests didn't match conformance test expectations)
danieljbruce
left a comment
There was a problem hiding this comment.
Overall the migration to typescript looks great and there are a few thoughts I would mention and post before doing the rest of the review.
-
We can just skip the Service Path tests and then the system-test CI check should pass. I think before these tests were just skipped anyways and used as a handy tool to be sure that the universe domain feature was working correctly.
-
Apart from that I think we should add TestReadRows_NoRetry_OutOfOrderError, TestReadRows_Generic_CloseClient and TestReadRows_Generic_DeadlineExceeded tests to the known_failures.txt list so that the CI check for mandatory conformance turns green. This tool is in place to make it really easy for us to determine exactly what we need to fix next.
danieljbruce
left a comment
There was a problem hiding this comment.
Overall the test proxy modernization looks really good. Outside of that there are a few comments about some of the proxy changes and source code changes we should discuss, but after those are resolved this should be good for approval.
|
|
||
| if (bigtable) { | ||
| await bigtable[v2].close(); |
There was a problem hiding this comment.
I don't know if we want to remove this. I notice that TestReadRows_Generic_CloseClient is now failing so this may be causing a regression in the conformance tests so I would consider adding the following lines back in:
await bigtable[v2].close();
clientMap.delete(clientId);
There was a problem hiding this comment.
...this might've been a mistake on my part. Thanks!
|
|
||
| const v2 = Symbol.for('v2'); | ||
|
|
||
| export function createBigtableClient(bigtable: Bigtable) { |
There was a problem hiding this comment.
nit: If this createBigtableClient method is used in only one place then I would prefer to keep this method in the file where it is used to encapsulate things a little more.
Same idea with getBigtableClient.
There was a problem hiding this comment.
It turns out that was a mistake anyway, related to the close bugs. This is now used in two other files. The reason I'd moved it out like this is just to make the create/close logic closer together.
|
|
||
| if (bigtable) { | ||
| await bigtable[v2].close(); |
There was a problem hiding this comment.
Should we delete this line and clientMap.delete(clientId); line? They might be causing a regression making the TestReadRows_Generic_CloseClient test fail.
This reverts commit 76ac50c.
This reverts commit f18d7b2.
…ix TestReadRow_Generic_DeadlineExceeded"" This reverts commit 86c535a.
…ejs-bigtable into conformance-fixes
Description
This fixes a couple of the conformance test failures, as well as: