Skip to content

Commit

Permalink
add system test, remove async on terminate
Browse files Browse the repository at this point in the history
  • Loading branch information
Brian Chen committed Dec 19, 2019
1 parent 2962654 commit 95a82b9
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 2 deletions.
4 changes: 2 additions & 2 deletions dev/src/index.ts
Expand Up @@ -1012,8 +1012,8 @@ export class Firestore {
*
* @return A Promise that resolves when the client is terminated.
*/
async terminate(): Promise<void> {
await this._clientPool.terminate();
terminate(): Promise<void> {
return this._clientPool.terminate();
}

/**
Expand Down
18 changes: 18 additions & 0 deletions dev/system-test/firestore.ts
Expand Up @@ -127,6 +127,24 @@ describe('Firestore class', () => {
expect(docs[1].data()).to.deep.equal({f: 'a'});
});
});

it('cannot make calls after the client has been terminated', () => {
const ref1 = randomCol.doc('doc1');
ref1.onSnapshot(snapshot => {
return Promise.reject('onSnapshot() should be called');

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Dec 19, 2019

Contributor

Should this be “Shouldn’t be called”?

Do note that this test is racey. It could indeed be called once before terminate() is processed. The saving grace here is that returning a rejected Promise here won’t fail the test - but it will result in a warning about an uncaught Promise. I think the best way to do this is using Mocha’s “done” function that you can invoke with an error once terminate is called.

});
return firestore
.terminate()
.then(() => {
return ref1.set({foo: 100});
})
.then(() => {
Promise.reject('set() should have failed');
})

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Dec 19, 2019

Contributor

The curly braces here mean that the rejected Promise won’t be returned. The test will pass but may log an uncaught Promise warning

.catch(err => {
expect(err.message).to.equal('The client has already been terminated');
});
});
});

describe('CollectionReference class', () => {
Expand Down

0 comments on commit 95a82b9

Please sign in to comment.