WIP: refactor(database): Add migrated test harness [Part 3/3] #71

jshcrowthe · 2017-06-19T23:39:19Z

Migrate the old test harness to the repo

jshcrowthe · 2017-06-20T00:01:47Z

This PR presumes that #62 has been merged first.

All integration tests pass at this point

…files

* refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements

There are some tests that have weird timing issues, and because of the polling nature of the original implementation, we never caught the issue. These should be resolved when able

…l test meaning

mikelehen

Some random feedback on EventAccumulator in case you want it. :-) I also spot-checked the tests a little bit, but didn't do a deep comparison between the old and new tests. In general, the approach looks good to me though! Thanks.

mikelehen · 2017-06-20T21:56:42Z

tests/database/helpers/EventAccumulator.ts

+  public eventData = [];
+  public promise;
+  public resolve;
+  public reject;


Are you intentionally not typing these? I would have assumed we'd include types for any new code we write...

Not intentional no. Just was more focused on nailing the test meaning over the helper implementation.

mikelehen · 2017-06-20T22:09:36Z

tests/database/helpers/EventAccumulator.ts

+  public resolve;
+  public reject;
+  constructor(private expectedEvents: number) {
+    if (!this.expectedEvents) throw new Error('EventAccumulator:You must pass a number of expected events to the constructor');


FWIW, the compiler will require you to pass a (number) argument, so this check shouldn't be necessary... unless you're actually trying to check for passing 0, in which case I would do an explicit === 0 check.

Also, I'm not sure if just counting events will work for many of the existing waitsFor() use cases. I suspect you'll need to be able to provide a closure that inspects the event or similar.

Some of the tests used the condition checking that you mentioned before, but the events were predictable enough that I just used the counts. If you feel we need the condition based then I'll prob refactor accordingly.

"predictable enough" makes me a bit nervous. If you're confident the counts are predictable then it's fine, but there are definitely cases where there could be races. E.g. if you write on one connection and listen on another, you may get an initial empty event first, or you may get the written data first, depending on which one hits the server first.

"predictable enough" means it really only has the possibility to vary if there is more than just the test suite hitting the server. It would probably be smart for us to find a way to isolate these tests from the firebase server somehow. That would remove a lot of moving parts.

mikelehen · 2017-06-20T22:13:56Z

tests/database/helpers/EventAccumulator.ts

+      this.reject = reject;
+    });
+  }
+  addEvent(eventData?: any) {


FWIW, "any" is considered dangerous and should generally be avoided. You could use a more specific type ("object" or "object|number|string" or something), but even better might be to make EventAccumulator generic (i.e. make it "EventAccumulator<T>", this method would take a "eventData: T" and this.promise would be a Promise<T[]>, etc. Then the events are typed all the way through. That's definitely how I'd write it from scratch, but if it complicates the migration, it's okay to do something easier.

mikelehen · 2017-06-20T22:22:03Z

tests/database/helpers/EventAccumulator.ts

+    this.eventData = [
+      ...this.eventData,
+      eventData
+    ];


Hrm. Why isn't this "this.eventData.push(eventData)" ?

Effectively the same thing. I have been in a habit of doing immutable arrays for a while. No big deal either way to me.

I would prefer to use push() here since it is easier to read.

mikelehen · 2017-06-20T22:27:08Z

tests/database/helpers/EventAccumulator.ts

+      eventData
+    ];
+    if (this.eventData.length >= this.expectedEvents) {
+      this.resolve(this.eventData);


What if this gets hit multiple times? I'm not sure if resolve() will ignore or throw... My guess is you'll want it to ignore because that's what waitsFor() would essentially do (since it stops polling once the condition is met)

A promise can only resolve once, so if it gets hit multiple times, only the first invocation will actually trigger a call to .then. All other invocations are ignored.

mikelehen · 2017-06-20T22:29:22Z

tests/database/helpers/EventAccumulator.ts

+    }
+  }
+  reset(expectedEvents?: number) {
+    this.expectedEvents = expectedEvents || this.expectedEvents;


FWIW, it might be cleaner to have a .waitFor(events: number): Promise method instead of this (and drop the number from the constructor).

And you could potentially have a .waitForCount(count: number) and .waitForEventMatching(condition: (event: T => boolean)) versions depending on whether you want to wait for a specific number, or wait until a condition is true, etc.

Fair enough! We could refactor to add that sure. I only ever used the event counter but there were a couple cases where we could leverage the event matching if we wanted.

The tests pass as it stands, but they aren't as flexible as allowing for a condition might be.

mikelehen · 2017-06-20T22:33:35Z

tests/database/query.test.ts

+   * We are actually hitting a server during these tests, due to
+   * the increased potential latency, adding a little extra to 
+   * the timeout counter to help counteract tests that may seem
+   * flaky due to network conditions


nit: This is a very run-on-y sentence. :-)

mikelehen · 2017-06-20T22:42:14Z

tests/database/query.test.ts

+        expect(childNames).to.deep.equal(['Walker', 'Michael']);
+        done();
+      });
+    });


did you intentionally un-promisify this? :-)

I only refactored completely those functions that chained runs/waitsFor blocks. If there was a path were there was a smaller code delta (in this case the function waited for an assignment of a truthy value to done) then I favored that.

This is something we could clean up as well if we wanted and I'm totally in favor of this. Just already a lot of code change and wanted to make review easy where I could.

googlebot · 2017-06-27T22:59:54Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

schmidt-sebastian · 2017-06-29T01:13:51Z

src/database/core/view/filter/RangedFilter.ts

@@ -1,6 +1,6 @@
 import { IndexedFilter } from "./IndexedFilter";
 import { PRIORITY_INDEX } from "../../../core/snap/indexes/PriorityIndex";
-import { NamedNode } from "../../../core/snap/Node";
+import { Node, NamedNode } from "../../../core/snap/Node";


This doesn't seem to be needed.

Typescript throws an "Unknown Symbol" compiler error if it isn't present :(

schmidt-sebastian · 2017-06-29T01:14:08Z

src/database/core/util/util.ts

@@ -114,7 +114,7 @@ export const buildLogMessage_ = function(var_args) {
 * Use this for all debug messages in Firebase.
 * @type {?function(string)}
 */
-export var logger = console.log.bind(console);
+export var logger = null;


Intentional?

Yes, the value is originally null. I set it because I was using the logging everywhere.

schmidt-sebastian · 2017-06-29T01:15:19Z

src/database/core/util/SortedMap.ts

@@ -714,35 +714,35 @@ export class SortedMap {
   * @param {(function(K, V):T)=} opt_resultGenerator
   * @return {SortedMapIterator.<K, V, T>} The iterator.
   */
-  getIterator(opt_resultGenerator) {
+  getIterator(resultGnerator?) {


Typo (here and everywhere)

I'll get it fixed.

schmidt-sebastian · 2017-06-29T01:22:13Z

tests/database/browser/crawler_support.test.ts

+    // This test mostly exists to make sure restRef really is using ReadonlyRestClient
+    // and we're not accidentally testing a normal Firebase connection. It also can
+    // be a little slow so adding an extra timeout to help out.
+    this.timeout(3500);


I would prefer that we set a global timeout for these tests rather than individual ones.

Agreed, I'll update this

schmidt-sebastian · 2017-06-29T17:53:46Z

tests/database/helpers/events.ts

@@ -31,7 +31,7 @@ function rawPath(firebaseRef) {
 * @param {string=} opt_helperName
 * @return {{waiter: waiter, watchesInitializedWaiter: watchesInitializedWaiter, unregister: unregister, addExpectedEvents: addExpectedEvents}}
 */
-export function eventTestHelper(pathAndEvents, helperName?, eventAccumulator?: EventAccumulator) {
+export function eventTestHelper(pathAndEvents, helperName?, eventAccumulator?: EventAccumulator | EventAccumulator[]) {


This could be converted to a vararg.

schmidt-sebastian · 2017-06-29T17:55:51Z

tests/database/helpers/events.ts

@@ -166,6 +189,7 @@ export function eventTestHelper(pathAndEvents, helperName?) {
    actualPathAndEvents.splice(actualPathAndEvents.length - initializationEvents, initializationEvents);
    initializationEvents = 0;

+    initResolve();


Nit: This is more of a "resolveInit()", since initX sounds like it would initialize X.

schmidt-sebastian · 2017-07-06T19:55:11Z

My comments look to be addressed in your reviews. Do you want to merge this PR as well? I think we still need to be some time making this more TypeScript like, but I don't want to block progress on further database development.

jshcrowthe · 2017-07-06T19:56:20Z

Per convo with @schmidt-sebastian we are going to merge this and will address any lasting issues before merge to master.

@schmidt-sebastian

* refactor(database): remove old database implementation [Part 1/3] (#61) * refactor(database): remove old database implementation This is part #1 of 4 PR's to migrate database * refactor(database): remove database build processes * refactor(database): Add typescript database implementation [Part 2/3] (#62) * refactor(database): add typescript implementation * refactor(database): update build process to include database.ts All integration tests pass at this point * refactor(*): refactor environment builds to be based on separate .ts files * WIP: patch database code in nodeJS * refactor(database): classes for typescript database implementation (#55) * refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements * fix(database): Add missing "typeof" (#74) https://github.com/firebase/firebase-js-sdk/blob/fd0728138d88c454f8e38a78f35d831d6365070c/src/database/js-client/core/Repo.js#L86 * WIP: fixes from @schmidt-sebastian's review * WIP: fix: TS Build error * fix(database): fix issue with missing repo method * WIP: review adjustments #1 * WIP: review comments #2 * WIP: refactor(database): Add migrated test harness [Part 3/3] (#71) * refactor(database): add typescript implementation * refactor(database): update build process to include database.ts All integration tests pass at this point * refactor(*): refactor environment builds to be based on separate .ts files * WIP: patch database code in nodeJS * refactor(database): classes for typescript database implementation (#55) * refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements * WIP: add the /tests/config dir to the .gitignore * WIP: add test harness * WIP: add query tests There are some tests that have weird timing issues, and because of the polling nature of the original implementation, we never caught the issue. These should be resolved when able * WIP: add database.test.ts * WIP: add node.test.ts * WIP: add sortedmap.test.ts * WIP: add datasnapshot.test.ts * WIP: add sparsesnapshottree.test.ts * refactor(database): refactor query.test.ts to better preserve original test meaning * WIP: add crawler_support.test.ts * WIP: refactor EventAccumulator.ts for data.test.ts * WIP: fix issue with query.test.ts * WIP: add connection.test.ts * WIP: add info.test.ts * WIP: add order_by.test.ts I only migrated some of these tests as there was a dependency on the server for several tests * WIP: fix several code signature problems, add test files * WIP: add transaction.test.ts * WIP: working on the broken npm test command * WIP: working on fixes * WIP: remove logging * WIP: fix node tests * fix(*): fixing test files and CI integration * WIP: tEMP: Allow branch builds * WIP: escape string * refactor(CI): use ChromeHeadless launcher * WIP: fixes from review. * WIP: skip flakey test * WIP: remove unneeded debugger statement * WIP: fixing nits * Prevent using uninitialized array in EventEmitter (#85) * perf(*): fixing build size output issues * chore(*): remove unneeded build deps

jshcrowthe added the api: database label Jun 19, 2017

jshcrowthe assigned schmidt-sebastian Jun 19, 2017

jshcrowthe requested a review from schmidt-sebastian June 19, 2017 23:39

google-oss-bot added the needs-triage label Jun 19, 2017

jshcrowthe changed the base branch from master to ts-database/final June 19, 2017 23:39

jshcrowthe and others added 5 commits June 20, 2017 15:25

refactor(database): add typescript implementation

4a0df56

refactor(database): update build process to include database.ts

dcf2f5c

All integration tests pass at this point

refactor(*): refactor environment builds to be based on separate .ts …

7e609d6

…files

WIP: patch database code in nodeJS

e72bfbe

refactor(database): classes for typescript database implementation (#55)

2f6a406

* refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements

jshcrowthe mentioned this pull request Jun 20, 2017

refactor(database): Implement database in Typescript #72

Merged

jshcrowthe force-pushed the ts-database/migrate-test-harness branch from 0414ce2 to 8d4fd92 Compare June 20, 2017 22:30

jshcrowthe added 10 commits June 20, 2017 15:30

WIP: add the /tests/config dir to the .gitignore

19519ac

WIP: add test harness

2758ca7

WIP: add query tests

7f78f89

There are some tests that have weird timing issues, and because of the polling nature of the original implementation, we never caught the issue. These should be resolved when able

WIP: add database.test.ts

b12a61f

WIP: add node.test.ts

a046261

WIP: add sortedmap.test.ts

622fed8

WIP: add datasnapshot.test.ts

d35b183

WIP: add sparsesnapshottree.test.ts

c81583d

refactor(database): refactor query.test.ts to better preserve origina…

f4fc618

…l test meaning

WIP: add crawler_support.test.ts

342d527

jshcrowthe force-pushed the ts-database/migrate-test-harness branch from 8d4fd92 to 342d527 Compare June 20, 2017 22:31

mikelehen reviewed Jun 20, 2017

View reviewed changes

jshcrowthe removed the needs-triage label Jun 20, 2017

jshcrowthe added 4 commits June 21, 2017 14:47

WIP: refactor EventAccumulator.ts for data.test.ts

c51999a

WIP: fix issue with query.test.ts

c0eba9c

WIP: add connection.test.ts

e290f67

WIP: add info.test.ts

c0af997

WIP: add transaction.test.ts

379a263

jshcrowthe force-pushed the ts-database/migrate-test-harness branch from 68ac56c to 379a263 Compare June 26, 2017 18:25

firebase deleted a comment from googlebot Jun 26, 2017

jshcrowthe added 4 commits June 27, 2017 10:45

WIP: working on the broken npm test command

3193651

WIP: working on fixes

d5f99d8

WIP: remove logging

58f8bff

WIP: fix node tests

869b9a5

jshcrowthe added 4 commits June 27, 2017 17:08

fix(*): fixing test files and CI integration

ebe491c

WIP: tEMP: Allow branch builds

2b1efa4

WIP: escape string

c40a189

refactor(CI): use ChromeHeadless launcher

d78d6c4

jshcrowthe mentioned this pull request Jun 28, 2017

refactor(database): Add types to several classes and utility methods #66

Merged

Merge branch 'ts-database/final' into ts-database/migrate-test-harness

2939025

jshcrowthe force-pushed the ts-database/migrate-test-harness branch from 4add144 to 2939025 Compare June 28, 2017 23:26

schmidt-sebastian reviewed Jun 29, 2017

View reviewed changes

jshcrowthe added 3 commits June 29, 2017 10:26

WIP: fixes from review.

d214ba0

WIP: skip flakey test

dc5c374

WIP: remove unneeded debugger statement

71ff51c

schmidt-sebastian reviewed Jun 29, 2017

View reviewed changes

WIP: fixing nits

2531901

schmidt-sebastian closed this Jul 6, 2017

jshcrowthe reopened this Jul 6, 2017

jshcrowthe merged commit 890207a into ts-database/final Jul 6, 2017

jshcrowthe deleted the ts-database/migrate-test-harness branch July 6, 2017 19:56

firebase locked and limited conversation to collaborators Oct 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: refactor(database): Add migrated test harness [Part 3/3] #71

WIP: refactor(database): Add migrated test harness [Part 3/3] #71

jshcrowthe commented Jun 19, 2017

jshcrowthe commented Jun 20, 2017

mikelehen left a comment

mikelehen Jun 20, 2017

jshcrowthe Jun 20, 2017

mikelehen Jun 20, 2017

mikelehen Jun 20, 2017

jshcrowthe Jun 20, 2017

mikelehen Jun 20, 2017

jshcrowthe Jun 21, 2017

mikelehen Jun 20, 2017 •

edited

Loading

mikelehen Jun 20, 2017

jshcrowthe Jun 20, 2017

schmidt-sebastian Jun 29, 2017

mikelehen Jun 20, 2017

jshcrowthe Jun 20, 2017

mikelehen Jun 20, 2017

jshcrowthe Jun 20, 2017

mikelehen Jun 20, 2017

mikelehen Jun 20, 2017

jshcrowthe Jun 20, 2017

googlebot commented Jun 27, 2017

schmidt-sebastian Jun 29, 2017

jshcrowthe Jun 29, 2017

schmidt-sebastian Jun 29, 2017

jshcrowthe Jun 29, 2017

schmidt-sebastian Jun 29, 2017

jshcrowthe Jun 29, 2017

schmidt-sebastian Jun 29, 2017

jshcrowthe Jun 29, 2017

schmidt-sebastian Jun 29, 2017

schmidt-sebastian Jun 29, 2017

jshcrowthe Jul 6, 2017

schmidt-sebastian commented Jul 6, 2017

jshcrowthe commented Jul 6, 2017

WIP: refactor(database): Add migrated test harness [Part 3/3] #71

WIP: refactor(database): Add migrated test harness [Part 3/3] #71

Conversation

jshcrowthe commented Jun 19, 2017

jshcrowthe commented Jun 20, 2017

mikelehen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen Jun 20, 2017 • 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

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

googlebot commented Jun 27, 2017

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

schmidt-sebastian commented Jul 6, 2017

jshcrowthe commented Jul 6, 2017

mikelehen Jun 20, 2017 •

edited

Loading