Enable firestore sdk to talk to emulator #1007

tonymeng · 2018-07-13T01:29:50Z

This change enables the testing sdk to talk to the firestore emulator. This is based off of https://docs.google.com/document/d/1dWAVm8-jK_JLsCOx4uZBj2MN_6auc7ood-0llWoojs8/edit?ts=5b47bec0#heading=h.sm63cr1eg2l2

tonymeng · 2018-07-13T01:31:58Z

packages/testing/src/api/index.ts

 import * as admin from 'firebase-admin';
 import request from 'request-promise';
 import * as fs from 'fs';
+import { FirebaseApp } from '@firebase/app-types';
+import * as util from '@firebase/util';


is there a better way to import these?

ryanpbrewster · 2018-07-13T03:29:04Z

packages/firestore/src/api/credentials.ts

@@ -179,7 +182,13 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
    const initialUserCounter = this.userCounter;
    const forceRefresh = this.forceRefresh;
    this.forceRefresh = false;
-    return (this.app as _FirebaseApp).INTERNAL.getToken(forceRefresh).then(
+    var token: Promise<FirebaseAuthTokenData>;


Do we have to modify the Firestore SDK in order to get this to work? I'd prefer not to introduce this tokenOverride value.

AFAICT, we'd have to change app-types to export FirebaseAppImpl or to move INTERNAL into FirebaseApp to give visibility to the testing package. If that's the only way to do this, I'd rather modify the firestore package than app package

ryanpbrewster · 2018-07-13T03:29:36Z

packages/testing/src/api/index.ts

+      tokenOverride: fakeToken
+    },
+    'app-' + new Date().getTime() + "-" + Math.random()
+  );


Can we initialize the app here, then immediately hijack the INTERNAL.getToken function?

merlinnot · 2018-07-13T16:58:00Z

I’m soooo happy to see this PR! You can’t even imagine!

ryanpbrewster

run npm test and make sure the linter likes your code. I think it made me change anything that wasn't mutable into a const declaration (instead of var).

ryanpbrewster · 2018-07-13T18:09:54Z

packages/testing/src/api/index.ts

+  }
+  if (typeof options.auth != 'object') {
+    throw new Error('auth must be an object');
+  }


If you want, you could declare the type for options (instead of leaving it as any) and let the compiler construct these assertions for you.

ryanpbrewster · 2018-07-13T18:10:50Z

packages/testing/src/api/index.ts

+    },
+    'app-' + new Date().getTime() + '-' + Math.random()
+  );
+  (app as any).INTERNAL.getToken = () =>


can you add a comment here explaining that this INTERNAL.getToken hijack is to inject the predefined authorization headers into the app (thus bypassing FirebaseAuth)?

ryanpbrewster · 2018-07-13T18:12:04Z

Also please don't submit this until at least one of the other reviewers LGTMs it.

schmidt-sebastian · 2018-07-13T21:02:30Z

packages/testing/src/api/index.ts

-  if (!('databaseName' in options)) {
-    throw new Error('databaseName not specified');
-  }
+export type TestAppOptions = {


I would suggest to make this an interface or to just inline it and use two arguments in initializeTestApp.

Almost :) But it looks fine the way it is.

schmidt-sebastian · 2018-07-13T21:06:36Z

packages/testing/src/api/index.ts

@@ -36,6 +39,10 @@ export function apps(): (admin.app.App | null)[] {
  return admin.apps;
 }

+export function firestoreApps(): (FirebaseApp | null)[] {


The distinction between Admin App, Test App and Firestore Test App is not very clear. Would it be possible to add an argument to initializeTestApp that allows us to turn on Firestore support?

I'm combining initializeTestApp and initializeFirestoreTestApp. though the backend support won't land until next week.

schmidt-sebastian · 2018-07-13T21:12:57Z

packages/testing/src/api/index.ts

-  }
+export type TestAppOptions = {
+  databaseName: string;
+  auth: Object;


Note that Object will only allow empty objects. You should use object.

https://blog.mariusschulz.com/2017/02/24/typescript-2-2-the-object-type

schmidt-sebastian · 2018-07-13T21:14:25Z

packages/testing/src/api/index.ts

+  ].join('.');
+  const app = firebase.initializeApp(
+    { projectId: options.projectId },
+    'app-' + new Date().getTime() + '-' + Math.random()


You can make this a little more intuitive if you add a local variable that will help let users know that this is the appName.

schmidt-sebastian · 2018-07-13T21:15:20Z

packages/testing/test/database.test.ts

@@ -108,13 +126,27 @@ describe('Testing Module Tests', function() {
    ).to.throw(/Could not find file/);
  });

-  it('apps() returns all created apps', async function() {
+  it('apps() returns all apps created except by initializeFirestoreTestApp', async function() {


This behavior is strange and unexpected.

cleaned it up

schmidt-sebastian · 2018-07-13T22:56:41Z

packages/testing/src/api/index.ts

+    appOptions = {
+      databaseURL: DBURL + '?ns=' + options.databaseName
+    };
+  } else if ('projectId' in options) {


Is there a reason to not support both projectId and databseURL? This code might be a little cleaner if you supported both, even if the interface doesn't allow for it.

schmidt-sebastian · 2018-07-13T22:59:02Z

packages/testing/src/api/index.ts

-  if (!('databaseName' in options)) {
-    throw new Error('databaseName not specified');
-  }
+export type TestAppOptions = {


Almost :) But it looks fine the way it is.

schmidt-sebastian · 2018-07-13T23:00:59Z

packages/testing/index.ts

@@ -26,5 +26,6 @@ export {
  assertSucceeds,
  initializeAdminApp,
  initializeTestApp,
+  initializeFirestoreTestApp,


You need to remove this to get the CI to pass.

…the testing sdk to no longer work with old versions of the RTDB emulator

jshcrowthe · 2018-07-16T19:04:55Z

packages/testing/package.json

@@ -1,6 +1,6 @@
 {
  "name": "@firebase/testing",
-  "version": "0.1.0",
+  "version": "1.0.0",


We don't need to do this, even though it is a breaking change major 0s allow for breaking changes between other versions also. Additionally, I update this at publish time so no need to edit.

@davideast

* Add @davideast as a CODEOWNER (#996) * Embed metadata directly into the RPC call (#979) * Embed metadata directly into the RPC call * [AUTOMATED]: Prettier Code Styling * Use ...args * [AUTOMATED]: Prettier Code Styling * Minimize diff * Add the OAuth assertion back in * Added missing type for optional database url. (#1001) * RxFire Realtime Database (#997) * initial database code * test setup * database tests * auditTrail and database tests * [AUTOMATED]: Prettier Code Styling * [AUTOMATED]: License Headers * Josh's comments. Database docs * [AUTOMATED]: Prettier Code Styling * Firestore docs * auth docs * declaration fixes * switch to peerDeps * [AUTOMATED]: Prettier Code Styling * test config * Expose array transforms and array contains queries. (#1004) Also remove test code that was combining multiple array contains queries since those were disallowed in 04c9c3a. * Move fieldFilter (free function) to Filter.create() (#988) This is a refactoring to unify filter creation across platforms. * Enable firestore sdk to talk to emulator (#1007) * Enable firestore sdk to talk to emulator * [AUTOMATED]: Prettier Code Styling * Revert firestore sdk changes * [AUTOMATED]: Prettier Code Styling * Revert credentials.ts * Cleanup * [AUTOMATED]: Prettier Code Styling * Set webSafe=false * Combine initializeTestApp and initializeFirestoreTestApp * [AUTOMATED]: Prettier Code Styling * Cleanup * [AUTOMATED]: Prettier Code Styling * Update major version since this is a breaking change that will cause the testing sdk to no longer work with old versions of the RTDB emulator * Completely remove admin sdk * Change version back to 0.1.0 * Setting GarbageSource in SyncEngine's constructor (#1010) * b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes. (#1014) This fixes an issue occurring when a limbo target receives a documentUpdate, then a global snapshot, and then a CURRENT. Because there was a global snapshot before the CURRENT, WatchChangeAggregator has no pending document updates and calls SyncEngine.targetContainsDocument() to see if we previously got any document from the backend for the target. See: https://github.com/firebase/firebase-js-sdk/blob/6905339235ad801291edc696dd75a08e80647f5b/packages/firestore/src/remote/watch_change.ts#L422 Prior to this change, targetContainsDocument() returned false because it relies on our Views to track the contents of the target, and we don't have Views for limbo targets. Thus WatchChangeAggregator incorrectly manufactures a NoDocument document update which deletes data from our cache. The fix is to have SyncEngine track the fact that we did indeed get a document for the limbo resolution and return true from targetContainsDocument(). * Updating yarn.lock * Add @firebase/util as a dep of @firebase/testing * Allow remote updates from watch to heal a cache with synthesized deletes in it (#1015) * Write a spec test for the busted cache * Modify spec test to demonstrate deletedDoc issue. (#1017) * Allow updates for targets where the document is modified * Fix getRemoteKeysForTarget() method name in comment. (#1020) While porting I noticed this was slightly wrong. targetContainsDocument() is the method in WatchChangeAggregator. The SyncEngine method I meant to reference is getRemoteKeysForTarget(). * Making sure we don't export 'experimental' (#1023) * Add a schema migration that drops the query cache (#1019) * Add a schema migration that drops the query cache This is a force fix for potential existence filter mismatches caused by firebase/firebase-ios-sdk#1548 The essential problem is that when resuming a query, the server is allowed to forget deletes. If the number of incorrectly synthesized deletes matches the number of server-forgotten deletes then the existence filter can give a false positive, preventing the cache from self healing. Dropping the query cache clears any client-side resume token which prevents a false positive existence filter mismatch. Note that the remote document cache and mutation queues are unaffected so any cached documents that do exist will still work while offline. * Implement review feedback * Add a release note for the fix to #1548 (#1024) * Ensure that we create an empty TargetGlobal row. (#1029) Ensure the v3 migration unconditionally creates the TargetGlobal row. Remove the no-longer-necessary v2 schema migration. * Remove unnecessary `any` (#1030) * Fix an errant any usage * [AUTOMATED]: Prettier Code Styling * Publish [email protected] * Unify local.QueryData with the other platforms (#1027) This makes it line up with it's own docs, and also the other platforms. * Fix to #1027 to allow SnapshotVersion == 0 (#1033) * Add iat to fake access token payload (#1022) * Add iat to fake access token payload * [AUTOMATED]: Prettier Code Styling * Simpler tests * [AUTOMATED]: Prettier Code Styling * Do not clobber iat * catch server error RESET_PASSWORD_EXCEED_LIMIT (#1037) * Merging Master into Multi-Tab (#1038)

tonymeng added 2 commits July 12, 2018 18:29

Enable firestore sdk to talk to emulator

808659f

[AUTOMATED]: Prettier Code Styling

b7d185b

tonymeng requested review from mikelehen, ryanpbrewster, schmidt-sebastian and wilhuff as code owners July 13, 2018 01:29

google-oss-bot added the needs-triage label Jul 13, 2018

tonymeng commented Jul 13, 2018

View reviewed changes

ryanpbrewster reviewed Jul 13, 2018

View reviewed changes

tonymeng added 3 commits July 13, 2018 11:06

Revert firestore sdk changes

0c7999c

[AUTOMATED]: Prettier Code Styling

254bf54

Revert credentials.ts

e7a155e

ryanpbrewster approved these changes Jul 13, 2018

View reviewed changes

tonymeng added 3 commits July 13, 2018 12:01

Cleanup

3b03a9c

[AUTOMATED]: Prettier Code Styling

d8865f7

Set webSafe=false

92977e8

schmidt-sebastian self-assigned this Jul 13, 2018

schmidt-sebastian suggested changes Jul 13, 2018

View reviewed changes

tonymeng added 2 commits July 13, 2018 15:26

Combine initializeTestApp and initializeFirestoreTestApp

3a2ff76

[AUTOMATED]: Prettier Code Styling

8452e3f

schmidt-sebastian approved these changes Jul 13, 2018

View reviewed changes

schmidt-sebastian reviewed Jul 13, 2018

View reviewed changes

tonymeng and others added 5 commits July 13, 2018 16:19

Cleanup

b2259f5

[AUTOMATED]: Prettier Code Styling

bf748c3

Update major version since this is a breaking change that will cause …

5497fba

…the testing sdk to no longer work with old versions of the RTDB emulator

Merge branch 'master' into tm/firestore-testing

d335a60

Completely remove admin sdk

585366f

jshcrowthe suggested changes Jul 16, 2018

View reviewed changes

Change version back to 0.1.0

85ca2e2

jshcrowthe approved these changes Jul 16, 2018

View reviewed changes

tonymeng merged commit f1a5e2e into master Jul 16, 2018

tonymeng deleted the tm/firestore-testing branch July 16, 2018 20:38

firebase locked and limited conversation to collaborators Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable firestore sdk to talk to emulator #1007

Enable firestore sdk to talk to emulator #1007

tonymeng commented Jul 13, 2018 •

edited

Loading

tonymeng Jul 13, 2018

ryanpbrewster Jul 13, 2018

tonymeng Jul 13, 2018

ryanpbrewster Jul 13, 2018

merlinnot commented Jul 13, 2018

ryanpbrewster left a comment

ryanpbrewster Jul 13, 2018

tonymeng Jul 13, 2018

ryanpbrewster Jul 13, 2018

tonymeng Jul 13, 2018

ryanpbrewster commented Jul 13, 2018

schmidt-sebastian Jul 13, 2018

tonymeng Jul 13, 2018

schmidt-sebastian Jul 13, 2018

schmidt-sebastian Jul 13, 2018

tonymeng Jul 13, 2018

schmidt-sebastian Jul 13, 2018

tonymeng Jul 13, 2018

schmidt-sebastian Jul 13, 2018

tonymeng Jul 13, 2018

schmidt-sebastian Jul 13, 2018

tonymeng Jul 13, 2018

schmidt-sebastian Jul 13, 2018

schmidt-sebastian Jul 13, 2018

schmidt-sebastian Jul 13, 2018

tonymeng Jul 16, 2018

jshcrowthe Jul 16, 2018 •

edited

Loading

tonymeng Jul 16, 2018

Enable firestore sdk to talk to emulator #1007

Enable firestore sdk to talk to emulator #1007

Conversation

tonymeng commented Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlinnot commented Jul 13, 2018

ryanpbrewster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanpbrewster commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshcrowthe Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonymeng commented Jul 13, 2018 •

edited

Loading

jshcrowthe Jul 16, 2018 •

edited

Loading