Add code for managing emulators and running database/firestore emulator tests via yarn commands. #1435

yifanyang · 2018-12-19T02:05:27Z

Refactor emulators managing code.

Exclude failing cases from firestore emulator tests.

ryanpbrewster · 2018-12-19T02:10:35Z

Can you provide a brief comment explaining how the excluded tests are failing? Some of these failures would be new to me.

yifanyang · 2018-12-19T02:15:13Z

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

-      owner: { name: 'Sebastian' },
-      'is.admin': true
-    };
+  (!persistence && USE_EMULATOR ? it.skip : it)(


(!persistence && USE_EMULATOR ? it.skip : it) is a bit cumbersome, but it seems to be the way we exclude tests currently like here or here. I wonder if we want to have some customized runner, like what we have for firestore unit test here.

Why does persistence come into play here? Also:

Please add comments (ideally in the form of something like "TODO(b/...): Disabled because emulator currently ..."

I don't trust my operator precedence assumptions, so if possible I'd throw some extra parents around (!persistence && USE_EMULATOR).

It is because that these tests failed only when persistence is false. I have categorized all the failure symptoms/messages in this doc. I can add some comments in code, but I'm not sure whether these failures are due to the sdk test itself or the emulator.

There is also a sample travis run that shows these failures (break down to each test file).

Hrm, I see. I think before moving forward we should try to understand all the failures. If there are things we know are expected to fail with the emulator right now, great, we can exclude them (if you do know of any specific expected issues right now, maybe mark them in the doc?). But I think it's probably worth spending some time investigating the rest. I may be able to take a look later this week. Else it'll be January.

I have tried to debug some failures, but couldn't tell whether it is because of the emulator itself, or we are calling the emulator in a wrong way, or there's more setup for the emulator that needs to be done before any test. I'll add more information I found in the doc.

So for this PR, do you think if it makes sense if I

remove code for 1) excluding any tests, 2) including firestore emulator test on Travis;

keep code for 1) starting/stoping emulators (a bit of refactoring), 2) yarn command to run database/firestore test with their emulators.

I want to have some partial result completed before the quarter ends, and it's probably easier for you to help debug failures this way.

Sure, that sounds great.

yifanyang · 2018-12-19T03:00:58Z

Here is the doc summarizing the failures, which are excluded in this pr. I'm not sure whether these failures are due to sdk test itself or emulator.

I wonder if this is the best way to blacklist failures. Or maybe I should create a hot list, so that we can track each failure separately?

mikelehen

+1 to understanding why all these tests need to be disabled...

mikelehen · 2018-12-19T22:48:32Z

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

-      owner: { name: 'Sebastian' },
-      'is.admin': true
-    };
+  (!persistence && USE_EMULATOR ? it.skip : it)(


Why does persistence come into play here? Also:

Please add comments (ideally in the form of something like "TODO(b/...): Disabled because emulator currently ..."

I don't trust my operator precedence assumptions, so if possible I'd throw some extra parents around (!persistence && USE_EMULATOR).

Add yarn commands for running database/firestore tests with their emulators.

yifanyang · 2018-12-20T03:29:07Z

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

 export const DEFAULT_SETTINGS = getDefaultSettings();
+console.info(`Default Settings: ${JSON.stringify(DEFAULT_SETTINGS)}`);


I see from tslint config that we don't want console.log. Do we allow console.info?

We prevent console.log / console.info because: 1) In the SDK, all logging should go through our official logging mechanism, 2) often developers accidentally leave debug console log statements in their PRs and this makes sure they don't get checked in.

So it's okay to use console logging in tests as long as it's used sparingly and it's intentional.

So in this case, I would just add a tslint suppression comment above it:

// tslint:disable-next-line:ban

mikelehen

Basically LGTM with a few nits.

mikelehen · 2018-12-20T17:57:04Z

scripts/emulator-testing/emulators/emulator.ts

+
+  constructor(port: number, namespace: string) {
+    this.port = port;
+    this.namespace = namespace;


namespace isn't used in this base class, and I think it only makes sense for database, so it should probably live in DatabaseEmulator.

mikelehen · 2018-12-20T17:58:41Z

scripts/emulator-testing/firestore-test-runner.ts

+
+import { FirestoreEmulator } from './emulators/firestore-emulator';
+
+async function runTest(port: number, namespace: string): Promise<any> {


namespace is really a database-only term. This should just be projectId.

mikelehen · 2018-12-20T18:00:45Z

scripts/emulator-testing/emulators/database-emulator.ts

+      'https://storage.googleapis.com/firebase-preview-drop/emulator/firebase-database-emulator-v3.5.0.jar';
+  }
+
+  async setPublicRules(): Promise<number> {


(possibly a question for @ryanpbrewster) Why don't we have similar rules for Firestore? Do the emulators have different default behavior for rules? Is that intentional?

They do have different behavior, though not for any particularly principled reason. I'm considering having the RTDB emulator start open-by-default.

Feiyang1 · 2018-12-20T19:52:40Z

scripts/emulator-testing/emulators/emulator.ts

+
+  async setUp(): Promise<void> {
+    return new Promise<void>((resolve, reject) => {
+      const promise: any = spawn(


Generally speaking, any is not allowed. Since typing is not available for child-process-promise, you can define the typing yourself as

interface ChildProcessPromise extends Promise<void> { childProcess: number; }

Feiyang1 · 2018-12-20T19:54:15Z

scripts/emulator-testing/emulators/emulator.ts

+    });
+  }
+
+  async tearDown(): Promise<void> {


async is not needed.

Feiyang1 · 2018-12-20T19:57:57Z

scripts/emulator-testing/emulators/firestore-emulator.ts

+    this.projectId = projectId;
+    this.binaryName = 'firestore-emulator.jar';
+    this.binaryUrl =
+      'https://storage.googleapis.com/firebase-preview-drop/emulator/cloud-firestore-emulator-v1.2.1.jar';


I'm a little concerned about the hard coded url. If we forgot to update the version, we will eventually become using an out dated emulator binary. @ryanpbrewster Is there any way to always get the latest binary?

There was some discussion before on specifying emulator versions.

The conclusion seemed to be that it's probably better to have a locked version and update the version manually, than to always get the latest version and have risk that test may fail surprisingly with the new version.

Okay. That sounds fine to me. Can you please add a comment about it and include a pointer to where the latest emulator can be found?

Feiyang1 · 2018-12-20T19:59:51Z

scripts/emulator-testing/database-test-runner.ts

+
+import { DatabaseEmulator } from './emulators/database-emulator';
+
+async function runTest(port: number, namespace: string): Promise<any> {


async is not needed.

Feiyang1 · 2018-12-20T20:00:31Z

scripts/emulator-testing/emulators/database-emulator.ts

+      'https://storage.googleapis.com/firebase-preview-drop/emulator/firebase-database-emulator-v3.5.0.jar';
+  }
+
+  async setPublicRules(): Promise<number> {


async is not needed.

Feiyang1 · 2018-12-20T20:00:40Z

scripts/emulator-testing/emulators/emulator.ts

+    this.port = port;
+  }
+
+  async download(): Promise<void> {


async is not needed.

Feiyang1 · 2018-12-20T20:00:55Z

scripts/emulator-testing/emulators/emulator.ts

+    });
+  }
+
+  async setUp(): Promise<void> {


async is not needed.

Feiyang1 · 2018-12-20T20:01:08Z

scripts/emulator-testing/firestore-test-runner.ts

+
+import { FirestoreEmulator } from './emulators/firestore-emulator';
+
+async function runTest(port: number, projectId: string): Promise<any> {


async is not needed.

yifanyang assigned mikelehen, ryanpbrewster and Feiyang1 Dec 19, 2018

google-oss-bot added the needs-triage label Dec 19, 2018

yifanyang requested review from bojeil-google, depoll, Feiyang1, gsoltis, hiranya911, mikelehen, rsgowman, schmidt-sebastian, var-const, wilhuff and zxu123 as code owners December 19, 2018 02:05

yifanyang commented Dec 19, 2018

View reviewed changes

mikelehen suggested changes Dec 19, 2018

View reviewed changes

yifanyang force-pushed the yifany-firestore-emulator-test branch from 4c0cf0e to 89e6203 Compare December 20, 2018 02:25

yifanyang changed the title ~~Include firestore emulator tests in Travis.~~ Add code for managing emulators and running database/firestore emulator tests via yarn commands. Dec 20, 2018

yifanyang force-pushed the yifany-firestore-emulator-test branch 3 times, most recently from d2b9268 to 2e8047d Compare December 20, 2018 03:18

Refactor code for managing database/firestore emulators.

df00b9f

Add yarn commands for running database/firestore tests with their emulators.

yifanyang force-pushed the yifany-firestore-emulator-test branch from 2e8047d to df00b9f Compare December 20, 2018 03:22

yifanyang commented Dec 20, 2018

View reviewed changes

Intentionally allow one console.log statement in test.

c6a2fd8

mikelehen approved these changes Dec 20, 2018

View reviewed changes

mikelehen removed their assignment Dec 20, 2018

address feedback

ba584ac

Feiyang1 reviewed Dec 20, 2018

View reviewed changes

Feiyang1 suggested changes Dec 20, 2018

View reviewed changes

address feedback

8cc5e62

yifanyang force-pushed the yifany-firestore-emulator-test branch from d7ffc77 to 8cc5e62 Compare December 20, 2018 20:21

fix

c10402b

Feiyang1 approved these changes Dec 20, 2018

View reviewed changes

yifanyang merged commit 54df997 into master Dec 20, 2018

yifanyang deleted the yifany-firestore-emulator-test branch December 20, 2018 23:00

firebase locked and limited conversation to collaborators Oct 14, 2019

		export const DEFAULT_SETTINGS = getDefaultSettings();
		console.info(`Default Settings: ${JSON.stringify(DEFAULT_SETTINGS)}`);


		import { FirestoreEmulator } from './emulators/firestore-emulator';

		async function runTest(port: number, namespace: string): Promise<any> {


		import { DatabaseEmulator } from './emulators/database-emulator';

		async function runTest(port: number, namespace: string): Promise<any> {


		import { FirestoreEmulator } from './emulators/firestore-emulator';

		async function runTest(port: number, projectId: string): Promise<any> {

Add code for managing emulators and running database/firestore emulator tests via yarn commands. #1435

Add code for managing emulators and running database/firestore emulator tests via yarn commands. #1435

Uh oh!

Conversation

yifanyang commented Dec 19, 2018

Uh oh!

ryanpbrewster commented Dec 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yifanyang commented Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yifanyang commented Dec 19, 2018 •

edited

Loading