Update the integration test to verify that bloom filter averted full requery #7095

dconeybe · 2023-03-07T03:57:49Z

Update the integration test for the bloom filter to also verify that Watch specified a bloom filter that could be used to avert the full requery.

Googlers see b/271205722 for more details.

Ported to Android in firebase/firebase-android-sdk#4768.

…loom filter without a false positive

changeset-bot · 2023-03-07T03:57:53Z

⚠️ No Changeset found

Latest commit: acff0f4

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 · 2023-03-07T04:19:20Z

Size Report ¹

Affected Products

`@firebase/firestore`

Type	Base (`5c2ec00`)	Merge (`1abd82c`)	Diff
browser	278 kB	279 kB	+988 B (+0.4%)
esm5	345 kB	346 kB	+1.14 kB (+0.3%)
main	555 kB	557 kB	+1.71 kB (+0.3%)
module	278 kB	279 kB	+988 B (+0.4%)
react-native	278 kB	279 kB	+988 B (+0.4%)

`bundle`

Type	Base (`5c2ec00`)	Merge (`1abd82c`)	Diff
firestore (Persistence)	296 kB	297 kB	+968 B (+0.3%)
firestore (Query Cursors)	234 kB	235 kB	+968 B (+0.4%)
firestore (Query)	232 kB	233 kB	+968 B (+0.4%)
firestore (Read data once)	219 kB	220 kB	+968 B (+0.4%)
firestore (Realtime updates)	221 kB	222 kB	+968 B (+0.4%)

firebase
Type Base (5c2ec00) Merge (1abd82c) Diff
firebase-compat.js 761 kB 762 kB +950 B (+0.1%)
firebase-firestore-compat.js 335 kB 336 kB +950 B (+0.3%)
firebase-firestore.js 337 kB 338 kB +988 B (+0.3%)

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/LhKnemWI25.html

google-oss-bot · 2023-03-07T04:32:01Z

Size Analysis Report ¹

This report is too large (530,201 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/zlUssNaxP7.html

…'t appear in common/api-review/firestore.api.md. Also do a tiny bit of cosmetic changes.

…ist of copied files This may fix the following error: ``` [tsl] ERROR in /home/runner/work/firebase-js-sdk/firebase-js-sdk/integration/firestore/temp/test/integration/api/query.test.ts(76,50) TS2307: Cannot find module '../util/testing_hooks_util' or its corresponding type declarations. ``` https://github.com/firebase/firebase-js-sdk/actions/runs/4351236611/jobs/7602696515

…smatchInfoFromRaw() to help debug this error: ``` TypeError: Cannot read properties of undefined (reading 'existenceFilter') ``` https://github.com/firebase/firebase-js-sdk/actions/runs/4355588983/jobs/7612412525

…terMismatch()' callbacks

…and/or excluded from minified code

…es minification

…y 'ignoreUndefinedProperties' does not exist on type '{ host: string; ssl: boolean; }' See https://github.com/firebase/firebase-js-sdk/actions/runs/4367447223/jobs/7638769445

… Property 'ignoreUndefinedProperties' does not exist on type '{ host: string; ssl: boolean; }'" This reverts commit c1437bc. Importing @firestore/types didn't work in CI... ugh.

…: Property 'ignoreUndefinedProperties' does not exist on type '{ host: string; ssl: boolean; }' See https://github.com/firebase/firebase-js-sdk/actions/runs/4367447223/jobs/7638769445

packages/firestore-compat/test/fields.test.ts

packages/firestore/src/util/testing_hooks.ts

packages/firestore/test/integration/api/query.test.ts

packages/firestore/test/integration/util/testing_hooks_util.ts

…erComplexIntegrationTest

dconeybe · 2023-03-09T20:09:51Z

@milaGGL I'm not sure why the "Test All Packages / Node.js and Browser (Chrome) Tests (push)" check is failing. It's failed for two completely different reasons in a row. Can you review despite that failure?

…unt to be less confusing, since I've already confused myself with this naming

…rgeting non-master branches. This is a cherry-pick of #7110

.github/workflows/check-changeset.yml

milaGGL · 2023-03-09T23:49:30Z

packages/firestore/test/integration/api/query.test.ts

+        // it sends ExistenceFilter messages. Some day the emulator _may_
+        // implement this logic, at which time this short-circuit can be
+        // removed.
+        if (USE_EMULATOR) {


can we remove the (USE_EMULATOR ? it.skip : it)( on line 2071, since we are passing the test here if it is emulator?

Instead of skipping the entire test I made a modification later on in the test to ignore the erroneous snapshot when the query is resumed. With that, the test can run, and pass, against the emulator. It doesn't really test much, but it at least tests what it can.

milaGGL · 2023-03-09T23:51:19Z

packages/firestore/test/integration/util/helpers.ts

@@ -192,7 +192,7 @@ export async function withTestDbsSettings(
  }

  try {
-    await fn(dbs);
+    return await fn(dbs);


If we return here, can it go to the finally block?

Yes, it will still execute the finally block.

…erComplexIntegrationTest

milaGGL

LGTM

dconeybe added 2 commits March 6, 2023 22:32

update the test case to verify that the existence filter contains a b…

86870f9

…loom filter without a false positive

add retry

f75534f

dconeybe added the api: firestore label Mar 7, 2023

dconeybe self-assigned this Mar 7, 2023

fix lint errors

dc42228

Update API reports

c1bff43

dconeybe added 16 commits March 7, 2023 00:22

testing_hooks.ts: add @internal tag to TestingHooks class so it doesn…

1753549

…'t appear in common/api-review/firestore.api.md. Also do a tiny bit of cosmetic changes.

Add docs

50418ef

fix lint errors

01b3bf0

cleanup

384608c

add a reference to b/271949433 in a comment

67305ea

get rid of unnecssary 'type IterationResult' declaration

f9e0707

fix mangling of the property names of the argument to 'onExistenceFil…

739f13d

…terMismatch()' callbacks

testing_hooks.ts: add @internal tag to ExistenceFilterMismatchInfo

af53f1a

avoid using objects for parameters since their names can get mangled …

85ed7ab

…and/or excluded from minified code

try hand-creating the ExistenceFilterMismatchInfo to see if it surviv…

0b9aaf1

…es minification

testing_hooks_util.ts: fix lint error

266a722

firestore-compat: fix typing error in fields.test.ts: TS2339: Propert…

c1437bc

…y 'ignoreUndefinedProperties' does not exist on type '{ host: string; ssl: boolean; }' See https://github.com/firebase/firebase-js-sdk/actions/runs/4367447223/jobs/7638769445

Revert "firestore-compat: fix typing error in fields.test.ts: TS2339:…

24fbb79

… Property 'ignoreUndefinedProperties' does not exist on type '{ host: string; ssl: boolean; }'" This reverts commit c1437bc. Importing @firestore/types didn't work in CI... ugh.

firestore-compat: *really* fix typing error in fields.test.ts: TS2339…

f9adb54

…: Property 'ignoreUndefinedProperties' does not exist on type '{ host: string; ssl: boolean; }' See https://github.com/firebase/firebase-js-sdk/actions/runs/4367447223/jobs/7638769445

dconeybe marked this pull request as ready for review March 9, 2023 01:59

dconeybe commented Mar 9, 2023

View reviewed changes

packages/firestore-compat/test/fields.test.ts Outdated Show resolved Hide resolved

dconeybe commented Mar 9, 2023

View reviewed changes

packages/firestore/src/util/testing_hooks.ts Outdated Show resolved Hide resolved

dconeybe commented Mar 9, 2023

View reviewed changes

packages/firestore/src/util/testing_hooks.ts Outdated Show resolved Hide resolved

dconeybe commented Mar 9, 2023

View reviewed changes

packages/firestore/test/integration/api/query.test.ts Outdated Show resolved Hide resolved

dconeybe commented Mar 9, 2023

View reviewed changes

packages/firestore/test/integration/api/query.test.ts Outdated Show resolved Hide resolved

dconeybe commented Mar 9, 2023

View reviewed changes

packages/firestore/test/integration/util/testing_hooks_util.ts Outdated Show resolved Hide resolved

dconeybe commented Mar 9, 2023

View reviewed changes

packages/firestore/test/integration/util/testing_hooks_util.ts Show resolved Hide resolved

dconeybe added 7 commits March 9, 2023 12:32

minor cleanups from self code review

def79d1

Merge remote-tracking branch 'origin/mila/BloomFilter' into BloomFilt…

6d4e3dd

…erComplexIntegrationTest

yarn prettier

f5415f5

watch_change.ts minor cleanup

6bc7b3b

query.test.ts: remove it.only()

bad0ca9

refactor the test a bit

9871614

query.test.ts: it.only -> it (oops)

61a9cd7

dconeybe requested a review from milaGGL March 9, 2023 20:09

dconeybe added 2 commits March 9, 2023 15:53

rename actualCount/expectedCount to localCacheCount/existenceFilterCo…

2acf945

…unt to be less confusing, since I've already confused myself with this naming

disable the annoying 'check changeset' check, which is broken when ta…

daf049c

…rgeting non-master branches. This is a cherry-pick of #7110

dconeybe requested review from dwyfrequency and hsubox76 as code owners March 9, 2023 20:56

dconeybe commented Mar 9, 2023

View reviewed changes

.github/workflows/check-changeset.yml Show resolved Hide resolved

milaGGL reviewed Mar 9, 2023

View reviewed changes

dconeybe added 3 commits March 9, 2023 20:33

Merge remote-tracking branch 'origin/mila/BloomFilter' into BloomFilt…

74607ac

…erComplexIntegrationTest

address review feedback

677b9e7

yarn prettier

acff0f4

milaGGL approved these changes Mar 10, 2023

View reviewed changes

dconeybe merged commit 7c20d7d into mila/BloomFilter Mar 10, 2023

dconeybe deleted the dconeybe/BloomFilterComplexIntegrationTest branch March 10, 2023 14:56

This was referenced Mar 10, 2023

Cleanup and improvements to the "complex" bloom filter integration test #7112

Merged

Update the integration test to verify that bloom filter averted full requery firebase/firebase-android-sdk#4768

Merged

firebase locked and limited conversation to collaborators Apr 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the integration test to verify that bloom filter averted full requery #7095

Update the integration test to verify that bloom filter averted full requery #7095

dconeybe commented Mar 7, 2023 •

edited

Loading

changeset-bot bot commented Mar 7, 2023 •

edited

Loading

google-oss-bot commented Mar 7, 2023 •

edited

Loading

`@firebase/firestore`

`bundle`

`firebase`

google-oss-bot commented Mar 7, 2023 •

edited

Loading

dconeybe commented Mar 9, 2023

milaGGL Mar 9, 2023

dconeybe Mar 10, 2023

milaGGL Mar 9, 2023

dconeybe Mar 10, 2023

milaGGL left a comment

Update the integration test to verify that bloom filter averted full requery #7095

Update the integration test to verify that bloom filter averted full requery #7095

Conversation

dconeybe commented Mar 7, 2023 • edited Loading

changeset-bot bot commented Mar 7, 2023 • edited Loading

⚠️ No Changeset found

google-oss-bot commented Mar 7, 2023 • edited Loading

Size Report 1

Affected Products

@firebase/firestore

bundle

firebase

Test Logs

google-oss-bot commented Mar 7, 2023 • edited Loading

Size Analysis Report 1

Test Logs

dconeybe commented Mar 9, 2023

milaGGL Mar 9, 2023

Choose a reason for hiding this comment

dconeybe Mar 10, 2023

Choose a reason for hiding this comment

milaGGL Mar 9, 2023

Choose a reason for hiding this comment

dconeybe Mar 10, 2023

Choose a reason for hiding this comment

milaGGL left a comment

Choose a reason for hiding this comment

dconeybe commented Mar 7, 2023 •

edited

Loading

changeset-bot bot commented Mar 7, 2023 •

edited

Loading

google-oss-bot commented Mar 7, 2023 •

edited

Loading

Size Report ¹

`@firebase/firestore`

`bundle`

`firebase`

google-oss-bot commented Mar 7, 2023 •

edited

Loading

Size Analysis Report ¹