Skip to content

Firestore: improve persistence parameter of integration tests to an object rather than a boolean #7404

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

Merged
merged 9 commits into from
Jul 4, 2023

Conversation

dconeybe
Copy link
Contributor

All of Firestore's integration tests are parameterized with a persistence boolean: true means to use indexeddb persistence with lru gc and false means to use memory persistence with eager gc.

But as of #6943 we now support memory persistence with lru gc. Some tests (specifically, resuming a query should use bloom filter to avoid full requery, which is undergoing this change in #7401) can take advantage of this new persistence mode so that it can be run with both memory and indexeddb persistence. However, the integration testing helper methods only use true/false as the persistence mode.

This PR generalizes the "persistence mode" from a boolean to a PersistenceMode object. This is mostly transparent to the tests as they mostly just "pass through" the persistence argument without really caring about its value. However, they were strongly typed as boolean. In this PR the tests were updated to basically ignore the type of the persistence argument, except where it mattered.

@dconeybe dconeybe self-assigned this Jun 28, 2023
@changeset-bot
Copy link

changeset-bot bot commented Jun 28, 2023

⚠️ No Changeset found

Latest commit: 5138e8a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 28, 2023

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 28, 2023

@dconeybe dconeybe marked this pull request as ready for review June 30, 2023 18:21
@dconeybe dconeybe requested review from a team as code owners June 30, 2023 18:21
@dconeybe dconeybe requested a review from milaGGL June 30, 2023 20:23
Copy link
Contributor

@milaGGL milaGGL left a comment

Choose a reason for hiding this comment

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

Beautiful! LGTM.

@dconeybe dconeybe merged commit 675ec3a into master Jul 4, 2023
@dconeybe dconeybe deleted the dconeybe/PersistenceTestGeneralization branch July 4, 2023 14:32
@firebase firebase locked and limited conversation to collaborators Aug 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants