Skip to content

Add an integration test for calling clearPersistence() on a new Firestore instance #3005

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 4, 2020
Merged

Add an integration test for calling clearPersistence() on a new Firestore instance #3005

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7bc5c93
Add a unit test for calling clearPersistence() on a newly-created Fir…
dconeybe Apr 29, 2020
1142137
Remove unused variable: firestore
dconeybe May 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion packages/firestore/test/integration/api/database.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1074,7 +1074,7 @@ apiDescribe('Database', (persistence: boolean) => {

// eslint-disable-next-line no-restricted-properties
(persistence ? it : it.skip)(
'can clear persistence if the client has not been initialized',
'can clear persistence if the client has been terminated',
async () => {
await withTestDoc(persistence, async docRef => {
const firestore = docRef.firestore;
Expand All @@ -1096,6 +1096,30 @@ apiDescribe('Database', (persistence: boolean) => {
}
);

// eslint-disable-next-line no-restricted-properties
(persistence ? it : it.skip)(
'can clear persistence if the client has not been initialized',
async () => {
await withTestDoc(persistence, async docRef => {
const firestore = docRef.firestore;
await docRef.set({ foo: 'bar' });
const app = docRef.firestore.app;
const name = app.name;
const options = app.options;

await app.delete();
const app2 = firebase.initializeApp(options, name);
const firestore2 = firebase.firestore!(app2);
await firestore2.clearPersistence();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the only difference between this test case and the one above it is that the location of the call to clearPersistence() has moved to be invoked on the newly-created firestore2 object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking - is this test meaningfully different from:

// eslint-disable-next-line no-restricted-properties
(persistence ? it : it.skip)( 'can clear persistence if the client has not been initialized',
  () => withTestDb(db => db.clearPersistence())
)

You should also update your PR title. You are writing a super fancy integration test, and not a unit test :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "difference" between that 1-line test is that it doesn't verify the side effects of clearPersistence(). Unfortunately, the extra lines all exist to provide the verification that clearPersistence() does indeed clear the persistence. Also, I will update the PR description as suggested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I did not quite catch that the first time around.

await firestore2.enablePersistence();
const docRef2 = firestore2.doc(docRef.path);
await expect(
docRef2.get({ source: 'cache' })
).to.eventually.be.rejectedWith('Failed to get document from cache.');
});
}
);

// eslint-disable-next-line no-restricted-properties
(persistence ? it : it.skip)(
'cannot clear persistence if the client has been initialized',
Expand Down