-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test initialization refactor #254
Conversation
Codecov Report
@@ Coverage Diff @@
## master #254 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 12 12
Lines 2104 2104
Branches 457 457
=====================================
Hits 2104 2104 Continue to review full report at Codecov.
|
87c07c6
to
8bde202
Compare
8bde202
to
f4cb701
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some significant caveats:
- I don't have enough context to really judge the high-level logic. I mainly checked that the code does implement that logic.
- the PR is huge and I might have easily missed something among the similar changes.
test/timestamp.js
Outdated
{timestampsInSnapshots: true}, DOCUMENT_WITH_TIMESTAMP) | ||
{ | ||
timestampsInSnapshots: true, | ||
keyFilename: './test/fake-certificate.json', |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
test/transaction.js
Outdated
assert.equal(request.type, 'query'); | ||
assert.deepEqual(actual, request.request); | ||
return request.stream; | ||
function runTransaction(callback, ...requests) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
}; | ||
|
||
return createInstance(overrides).then(firestore => { | ||
firestore._preferTransactions = true; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
projectId: PROJECT_ID, | ||
sslCreds: SSL_CREDENTIALS, | ||
timestampsInSnapshots: true, | ||
keyFilename: './test/fake-certificate.json', |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Changed base branch to master and added @stephenplusplus for owners approval. |
This PR is virtually unreviewable, but is meant to make the next PR reviewable.
The follow up PR will change the way that client initialization works. Currently, the Server SDK has a single member variable (_firestoreClient) that stores the Veneer client. All tests use _firestoreClient to to add request assertions.
In order to support more than 100 concurrent listeners, a follow up PR will change this and support an arbitrary number of clients. The tests will then no longer be able to rely on
_firestoreClient
and won't be able to override request handlers directly. Instead, they will override the logic that creates these clients: https://github.com/googleapis/nodejs-firestore/pull/242/files#diff-51dde1525cb3ea0c2eea78ebf10ea7e2R84While the second PR will add actual changes to the client, this PR just changes all of the tests to use a new helper method to init the client (look in
test/util/helpers.ts
). This required initialization and formatting changes all over the test code. These changes all follow the same pattern, and probably don't need to be reviewed in detail each time.There is one logic change in the client to make the test work: With the new initialization, some of the conformance tests no longer pass since they can't tell that two paths are equal if one already has
_formattedName
initialized. I got rid of the lazy-loading here.