Skip to content

Firestore: QueryTest.java: use a different Firestore instance instead of a WriteBatch #5184

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

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Jul 21, 2023

Use a different Firestore instance instead of a WriteBatch to avoid affecting the local cache in QueryTest.java.

I had originally thought that WriteBatch sidestepped the local cache, just like a transaction did; however, that turned out to be incorrect and using a WriteBatch nullified the test cases that were modified to use it. This is a follow-up to #5167.

This PR was ported from firebase/firebase-js-sdk#7486.

… of a WriteBatch to avoid affecting the local cache.

This fixes the bug introduced by #5167 where WriteBatch affects the local cache (which I didn't realize).
@github-actions
Copy link
Contributor

github-actions bot commented Jul 21, 2023

Release note changes

No release note changes were detected. If you made changes that should be
present in the next release, ensure you've added an entry in the appropriate
CHANGELOG.md file(s).

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 21, 2023

Coverage Report 1

Affected Products

  • firebase-firestore

    FilenameBase (2314e82)Merge (a95c793)Diff
    PatchMutation.java98.39%100.00%+1.61%
    SetMutation.java97.22%94.44%-2.78%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/0t6yOYaTbU.html

@github-actions
Copy link
Contributor

github-actions bot commented Jul 21, 2023

Unit Test Results

   162 files   -    656     162 suites   - 656   2m 22s ⏱️ - 35m 50s
1 164 tests  - 3 792  1 148 ✔️  - 3 787  16 💤  -   5  0 ±0 
2 328 runs   - 7 493  2 296 ✔️  - 7 483  32 💤  - 10  0 ±0 

Results for commit f2d25c0. ± Comparison against base commit 2314e82.

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 21, 2023

@dconeybe dconeybe marked this pull request as ready for review July 21, 2023 12:32
@dconeybe dconeybe requested a review from milaGGL July 21, 2023 12:33
@google-oss-bot
Copy link
Contributor

Startup Time Report 1

Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS.

Notes

Startup Times

  • fire-fst

    DeviceStatisticsDistributions
    oriole-32
    Percentile2314e82a95c793DiffSignificant (?)
    p10323 ±17 μs313 ±16 μs-10.0 μs (-3.1%)NO
    p25338 ±23 μs323 ±19 μs-14.3 μs (-4.2%)NO
    p50361 ±37 μs339 ±22 μs-22.2 μs (-6.1%)NO
    p75410 ±95 μs394 ±68 μs-16.4 μs (-4.0%)NO
    p90460 ±128 μs460 ±108 μs+628 ns (+0.1%)NO

    20 test runs in comparison
    CommitTest Runs
    2314e82
    • 2023-07-21_15:34:58.718810_jJkh
    • 2023-07-21_15:34:58.724344_GrQI
    • 2023-07-21_15:34:58.724478_Dwyf
    • 2023-07-21_15:34:58.724488_VCIm
    • 2023-07-21_15:34:58.724495_KIxr
    • 2023-07-21_15:34:58.724636_MivF
    • 2023-07-21_15:34:58.724758_VdUG
    • 2023-07-21_15:34:58.724766_yejo
    • 2023-07-21_15:34:58.724772_FKjv
    • 2023-07-21_15:34:58.724778_Ewec
    a95c793
    • 2023-07-21_16:52:59.729902_OiWa
    • 2023-07-21_16:52:59.732192_BDCa
    • 2023-07-21_16:52:59.732202_OWPB
    • 2023-07-21_16:52:59.732209_zexJ
    • 2023-07-21_16:52:59.732215_avsM
    • 2023-07-21_16:52:59.732221_lSiq
    • 2023-07-21_16:52:59.732334_YQKA
    • 2023-07-21_16:52:59.732342_jgjE
    • 2023-07-21_16:52:59.732347_gaVa
    • 2023-07-21_16:52:59.732352_HZaw
    redfin-30
    Percentile2314e82a95c793DiffSignificant (?)
    p10607 ±40 μs615 ±24 μs+8.57 μs (+1.4%)NO
    p25624 ±47 μs635 ±29 μs+11.5 μs (+1.8%)NO
    p50656 ±59 μs670 ±38 μs+14.0 μs (+2.1%)NO
    p75689 ±67 μs722 ±60 μs+32.4 μs (+4.7%)NO
    p90754 ±120 μs863 ±223 μs+108 μs (+14.4%)NO

    20 test runs in comparison
    CommitTest Runs
    2314e82
    • 2023-07-21_15:34:58.718810_jJkh
    • 2023-07-21_15:34:58.724344_GrQI
    • 2023-07-21_15:34:58.724478_Dwyf
    • 2023-07-21_15:34:58.724488_VCIm
    • 2023-07-21_15:34:58.724495_KIxr
    • 2023-07-21_15:34:58.724636_MivF
    • 2023-07-21_15:34:58.724758_VdUG
    • 2023-07-21_15:34:58.724766_yejo
    • 2023-07-21_15:34:58.724772_FKjv
    • 2023-07-21_15:34:58.724778_Ewec
    a95c793
    • 2023-07-21_16:52:59.729902_OiWa
    • 2023-07-21_16:52:59.732192_BDCa
    • 2023-07-21_16:52:59.732202_OWPB
    • 2023-07-21_16:52:59.732209_zexJ
    • 2023-07-21_16:52:59.732215_avsM
    • 2023-07-21_16:52:59.732221_lSiq
    • 2023-07-21_16:52:59.732334_YQKA
    • 2023-07-21_16:52:59.732342_jgjE
    • 2023-07-21_16:52:59.732347_gaVa
    • 2023-07-21_16:52:59.732352_HZaw
  • timeToInitialDisplay

    DeviceStatisticsDistributions
    oriole-32
    Percentile2314e82a95c793DiffSignificant (?)
    p10200 ±6 ms205 ±4 ms+4.20 ms (+2.1%)NO
    p25207 ±7 ms211 ±4 ms+3.92 ms (+1.9%)NO
    p50214 ±6 ms219 ±4 ms+4.39 ms (+2.0%)NO
    p75222 ±6 ms227 ±5 ms+5.19 ms (+2.3%)NO
    p90231 ±5 ms238 ±5 ms+7.34 ms (+3.2%)NO

    20 test runs in comparison
    CommitTest Runs
    2314e82
    • 2023-07-21_15:34:58.718810_jJkh
    • 2023-07-21_15:34:58.724344_GrQI
    • 2023-07-21_15:34:58.724478_Dwyf
    • 2023-07-21_15:34:58.724488_VCIm
    • 2023-07-21_15:34:58.724495_KIxr
    • 2023-07-21_15:34:58.724636_MivF
    • 2023-07-21_15:34:58.724758_VdUG
    • 2023-07-21_15:34:58.724766_yejo
    • 2023-07-21_15:34:58.724772_FKjv
    • 2023-07-21_15:34:58.724778_Ewec
    a95c793
    • 2023-07-21_16:52:59.729902_OiWa
    • 2023-07-21_16:52:59.732192_BDCa
    • 2023-07-21_16:52:59.732202_OWPB
    • 2023-07-21_16:52:59.732209_zexJ
    • 2023-07-21_16:52:59.732215_avsM
    • 2023-07-21_16:52:59.732221_lSiq
    • 2023-07-21_16:52:59.732334_YQKA
    • 2023-07-21_16:52:59.732342_jgjE
    • 2023-07-21_16:52:59.732347_gaVa
    • 2023-07-21_16:52:59.732352_HZaw
    redfin-30
    Percentile2314e82a95c793DiffSignificant (?)
    p10244 ±3 ms267 ±6 ms+22.9 ms (+9.4%)MAYBE
    p25252 ±3 ms274 ±8 ms+22.6 ms (+9.0%)MAYBE
    p50259 ±3 ms282 ±8 ms+24.0 ms (+9.3%)MAYBE
    p75267 ±4 ms294 ±10 ms+26.5 ms (+9.9%)NO
    p90278 ±7 ms309 ±9.9 ms+31.4 ms (+11.3%)MAYBE

    20 test runs in comparison
    CommitTest Runs
    2314e82
    • 2023-07-21_15:34:58.718810_jJkh
    • 2023-07-21_15:34:58.724344_GrQI
    • 2023-07-21_15:34:58.724478_Dwyf
    • 2023-07-21_15:34:58.724488_VCIm
    • 2023-07-21_15:34:58.724495_KIxr
    • 2023-07-21_15:34:58.724636_MivF
    • 2023-07-21_15:34:58.724758_VdUG
    • 2023-07-21_15:34:58.724766_yejo
    • 2023-07-21_15:34:58.724772_FKjv
    • 2023-07-21_15:34:58.724778_Ewec
    a95c793
    • 2023-07-21_16:52:59.729902_OiWa
    • 2023-07-21_16:52:59.732192_BDCa
    • 2023-07-21_16:52:59.732202_OWPB
    • 2023-07-21_16:52:59.732209_zexJ
    • 2023-07-21_16:52:59.732215_avsM
    • 2023-07-21_16:52:59.732221_lSiq
    • 2023-07-21_16:52:59.732334_YQKA
    • 2023-07-21_16:52:59.732342_jgjE
    • 2023-07-21_16:52:59.732347_gaVa
    • 2023-07-21_16:52:59.732352_HZaw

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/8WnfTeWBlO/index.html

@dconeybe dconeybe requested a review from milaGGL July 24, 2023 17:30
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.

LGTM

@dconeybe dconeybe merged commit 2827bac into master Jul 24, 2023
@dconeybe dconeybe deleted the dconeybe/BloomFilterTestUseDifferentInstanceInsteadOfWriteBatch branch July 24, 2023 19:26
@firebase firebase locked and limited conversation to collaborators Aug 24, 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