STITCH-1956 Fill in missing unit tests for every relevant task and clean exisiting tests. #69

jsflax · 2018-10-12T12:26:23Z

Do not review.

This is for agreement purposes only.

coveralls · 2018-10-12T14:35:52Z

Coverage increased (+12.0%) to 70.109% when pulling 23e6f64 on jsflax:STITCH-1956 into 94ea1fe on mongodb:master.

edaniels

Nice tests so far. Have some various suggestions to improve the strength, readability, and correctness of the tests

edaniels · 2018-10-12T15:22:50Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+            null,
+            BsonDocumentCodec())
+
+        dataSynchronizer.start()


[style] This takes up a lot of space. I would just group the operation you do on the element with the assertion right after. e.g.

dataSynchronizer.stop() dataSynchronizer.disableSyncThread() dataSynchronizer.start() assertFalse(dataSynchronizer.isRunning)

edaniels · 2018-10-12T15:28:43Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+
+    @Test
+    fun testNew() {
+        assertFalse(dataSynchronizer.isRunning)


[minor] For unit tests, it reads easier if they don't use a test setup method and instead call a method to get what they need. For example in this test, it's odd that all we check is isRunning but the user reading doesn't know where it comes from.

edaniels · 2018-10-12T15:29:16Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

    }

    @Test
    @Suppress("UNCHECKED_CAST")


Are these suppression necessary?

Removes unnecessary warnings due to us having to cast certain mocks.

edaniels · 2018-10-12T15:29:56Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+
+        dataSynchronizer.configure(
+            namespace,
+            mock(ConflictHandler::class.java) as ConflictHandler<BsonDocument>,


[nit] Instead of mocking these, just provide real ones. There's no value gained in mocking them as they are never manipulated.

edaniels · 2018-10-12T15:30:29Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

        authMonitor
    ))

+    private val dataSynchronizer = spy(DataSynchronizer(


[nit] Move this into a method where new versions of these can be fetched. That means so should the service, local client, and so on. It makes the tests less brittle.

edaniels · 2018-10-12T15:52:53Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+
+            dataSynchronizer.deleteOneById(namespace, document1["_id"])
+
+            `when`(coreRemoteMongoCollectionMock.deleteOne(any())).thenAnswer {


[test] For all tests that end up emitting errors, we should verify what the error is after each acquire.

edaniels · 2018-10-12T15:55:31Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+    }
+
+    @Test
+    fun testFailedDelete() {


[test] This test should verify we have frozen sync for this. It would also be good to test after doing insertOneAndSync again that it's not longer frozen and we insert.

edaniels · 2018-10-12T15:59:13Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+        dataSynchronizer.insertOneAndSync(namespace, document1)
+
+        // verify triggerListeningToNamespace has been called
+        verify(dataSynchronizer, times(1)).triggerListeningToNamespace(namespace)


I think this is looking too deep into the class. I would remove this and rely on stream function which is the true signal

edaniels · 2018-10-12T15:59:25Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+        dataSynchronizer.insertOneAndSync(namespace, document2)
+
+        // verify trigger is called again
+        verify(dataSynchronizer, times(3)).triggerListeningToNamespace(namespace)


Same here, can delete.

edaniels · 2018-10-12T15:59:39Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+        eventSemaphore.acquire()
+
+        // verify trigger is called, once for the insert, once for the configure
+        verify(dataSynchronizer, times(2)).triggerListeningToNamespace(namespace)


adamchel

This is looking pretty good and I definitely think it's going in the right direction. Had a few questions/minor comments, and will probably take a closer look once it's actually ready for review.

adamchel · 2018-10-12T19:54:16Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

   */
  public boolean doSyncPass() {
-    if (!syncLock.tryLock()) {
+    if (!this.isConfigured || !syncLock.tryLock()) {


[q] is this what you think might solve the flakiness we're seeing in the configure test?

No, this is actually how the method should function. I don't believe this will solve any races.

adamchel · 2018-10-12T20:27:05Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+            verify(conflictHandler, times(1)).resolveConflict(eq(document1["_id"]), any(), any())
+            verify(errorListener, times(0)).onError(eq(document1["_id"]), any())
+
+            // verify we are inserting!


[nit] verify we are not inserting? or that we desync-ed/froze?

or is this verifying that the conflict resolver deleted the document?

adamchel · 2018-10-12T20:33:04Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+            verify(conflictHandler, times(0)).resolveConflict(eq(document2["_id"]), any(), any())
+            verify(errorListener, times(1)).onError(eq(document2["_id"]), any())
+
+            assertEquals(


[minor] I think in general, for all these tests, we should add a comment when checking the document in the local collection to explain why we're checking for the specific expected state. I was confused at first why we were expecting the document to exist when the insert failed, but I realized it's because we expect the document to be there locally, just not remotely.

adamchel · 2018-10-12T20:34:51Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+            verify(conflictHandler, times(1)).resolveConflict(eq(document1["_id"]), any(), any())
+            verify(errorListener, times(0)).onError(eq(document1["_id"]), any())
+
+            // verify we are inserting!


inserting the resolution to the conflict?

adamchel · 2018-10-12T20:39:02Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+            return BsonDocument("_id", BsonObjectId()).append(key, value)
+        }
+
+        private fun withoutVersionId(document: BsonDocument?): BsonDocument? {


[heads up] this should be renamed to withoutSyncVersion to be consistent with the change I made in my PR

…into STITCH-1956

…platform independent

…into STITCH-1956

…coreDocConfig

jsflax · 2018-10-22T16:05:19Z

This is a large PR.

Add several tests for each of our core models (configs, ChangeEvents, etc.). These are mostly sanity checks and round trip checks.
Add a suite of unit tests to test the ins and outs of the DataSynchronizer, NamespaceChangeStreamListener, and CoreSync. These tests are managed by the SyncTestHarness, which contains a series of helper methods in DataSynchronizerTestContext. The test context should make testing easier to read and more manageable.
Consolidate android/server test logic. Proxy relevant methods through a core proxy and run the tests in core. See ProxyRemoteMethods, ProxySyncMethods, SyncIntTestProxy, SyncIntTestRunner.
Add comments to integration tests, making them more legible.
Fix testConfigure race in Int tests.
Various bug fixes.

@edaniels @adamchel PTAL.

Note: Seems like GitHub is having some issues. This is the patch for the latest commit on evg: https://evergreen.mongodb.com/version/5bcdf2dd2a60ed479842434d

edaniels

LGTM. Got a question or comment or two. Nice job.

edaniels · 2018-10-22T21:11:39Z

core/testutils/src/main/java/com/mongodb/stitch/core/testutils/sync/SyncIntTestRunner.kt

+
+    @Test
+    fun testStaleFetchMultiple()
+}


edaniels · 2018-10-22T21:14:10Z

core/testutils/src/main/java/com/mongodb/stitch/core/testutils/sync/SyncIntTestProxy.kt

+        // insert a document remotely, and wait for the event to store
+        val sem = watchForEvents(syncTestRunner.namespace)
+        remoteColl.insertOne(Document("_id", insertedId).append("fly", "away"))
+        sem.tryAcquire(3, TimeUnit.SECONDS)


[mahor] none of these tests that previously had just a acquire should have try acquire now. That encourages races to occur. If 3 seconds pass and nothing happens, this proceeds.

Fair point. It seemed to me that tryAcquire would at least let the test fail, as opposed to hang. I had only noticed it was an option as it was added to some of the more recent tests. However, I'll remove it from the tests I added it to. In this test, we actually do not need to acquire that semaphore at all– it can be refactored to only play with a changeEvent semaphore, an errorListener semaphore, and a conflict handling semaphore.

edaniels · 2018-10-22T21:14:34Z

...c/test/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/SyncTestHarness.kt

+        }
+
+        override fun waitForEvent() {
+            eventSemaphore?.tryAcquire(2, TimeUnit.SECONDS)


These should all change too. We shouldn't base any of these on time

edaniels · 2018-10-22T21:19:48Z

...c/test/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/SyncTestHarness.kt

@@ -0,0 +1,567 @@
+package com.mongodb.stitch.core.services.mongodb.remote.sync.internal


Should this have the word Unit in it to denote that it's only for unit tests?

I was under the impression that "harness" was a term generally for unit tests. Though Int tests have setup methods, you wouldn't want a harness per se.

On the contrary, adding the word Unit does make it undoubtably clear. So I will add it.

adamchel

Excellent work! This should make future testing a lot less painful. I have mostly nits and q's, some of which may be out of scope for this ticket anyway and are more just general questions I have about the overall implementation.

adamchel · 2018-10-22T21:48:28Z

.../stitch/core/services/mongodb/remote/sync/internal/NamespaceChangeStreamListenerUnitTests.kt

+        assertTrue(namespaceChangeStreamListener.openStream())
+        ctx.nextStreamEvent = Event.Builder().withEventName("message").build()
+        namespaceChangeStreamListener.storeNextEvent()
+        assertTrue(namespaceChangeStreamListener.isOpen)


[nit] add a check for assertEquals(0, namespaceChangeStreamListener.events.size)?

adamchel · 2018-10-22T22:17:07Z

...test/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/CoreSyncUnitTests.kt

+            coreSync.insertOneAndSync(ctx.testDocument)
+            fail("should have received duplicate key error index")
+        } catch (e: MongoWriteException) {
+            assertNotNull(e)


[nit] you can verify that this is duplicate key error by checking assertTrue(e.message.contains("E11000")) (or something similar)

adamchel · 2018-10-22T22:21:56Z

...c/test/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/SyncTestHarness.kt

+                BsonObjectId().value.toHexString())
+        }
+
+        fun withoutVersionId(document: BsonDocument?): BsonDocument? {


[nit] I renamed this to withoutSyncVersion in the integration tests since it's more than just an ID now, so you could change it here as well.

adamchel · 2018-10-22T22:25:05Z

...c/test/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/SyncTestHarness.kt

+            emitErrorSemaphore: Semaphore? = null,
+            expectedDocumentId: BsonValue? = null
+        ): ErrorListener {
+            open class TestErrorListener : ErrorListener {


[nit q] why is this class embedded in the function whereas the errors are at the level of the SyncTestHarness class?

At the moment, we don't need to interact with it. Might as well not muddle up what developers can access.

adamchel · 2018-10-22T22:29:11Z

...c/test/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/SyncTestHarness.kt

+        }
+
+        override fun waitForEvent() {
+            eventSemaphore?.tryAcquire(2, TimeUnit.SECONDS)


[minor] assertTrue(eventSemaphore?.tryAcquire(10, TimeUnit.SECONDS))
(or however you need to do it so the case where the semaphore is null works properly too).

adamchel · 2018-10-22T23:22:57Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+        // should be frozen since the operation type was unknown
+        ctx.queueConsumableRemoteUpdateEvent()
+        ctx.doSyncPass()
+        assertEquals(ctx.testDocument, ctx.findTestDocumentFromLocalCollection())


[nit q] same q as before, how do we know this is frozen? don't we need to try deleting first?

We don't need to try deleting– making sure the document doesn't update works as well.

adamchel · 2018-10-22T23:26:31Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

-            it.isFrozen = true
-        }
-        dataSynchronizer.reloadConfig()
+        // 2: Local wins


[nit] where is 1.?

Adding now.

adamchel · 2018-10-22T23:29:56Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+
+        ctx.doSyncPass()
+        ctx.waitForError()
+        // verify we are inserting!


[nit] copy-pasted comment?

adamchel · 2018-10-22T23:32:36Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+        // assert this doc does not exist
+        assertNull(ctx.findTestDocumentFromLocalCollection())
+
+        // update the non-existent document...


[nit] update -> delete

adamchel · 2018-10-22T23:36:03Z

core/testutils/src/main/java/com/mongodb/stitch/core/testutils/sync/SyncIntTestProxy.kt

+        // change event listener have been called
+        streamAndSync()
+
+        changeEventListenerSemaphore.tryAcquire(3, TimeUnit.SECONDS)


[major] another tryAcquire to assertTrue and change to 10 seconds

jsflax · 2018-10-24T15:30:36Z

@adamchel PTAL

adamchel

LGTM!

jsflax added 3 commits October 12, 2018 13:25

initial test draft

2408b2b

Add frozen check for fail cases

ca0cb95

Add delete cases

d864c35

edaniels suggested changes Oct 12, 2018

View reviewed changes

edaniels requested a review from adamchel October 12, 2018 16:00

adamchel reviewed Oct 12, 2018

View reviewed changes

jsflax added 15 commits October 19, 2018 15:08

Most cases complete; general progress commit

bfe2d95

creating context from test harness

0c8a879

Merge branch 'master' of https://github.com/mongodb/stitch-android-sdk …

fd41465

…into STITCH-1956

Clean up integration tests. Consolidate logic. Allow the tests to be …

0e4418d

…platform independent

Pre-lint pass

c53ecee

Refactor for lint

748196e

Core_Methods => Proxy_Methods

618d0b1

Merge branch 'master' of https://github.com/mongodb/stitch-android-sdk …

67643bd

…into STITCH-1956

Fix testConfigure race. Lint issues

ff9b6fd

Add comments to DataSynchronizerTestContext; add round trip test for …

279862e

…coreDocConfig

Fix overaggressive IDE refactor

8c23fe7

Add teardown for CoreSyncUnitTests

4fda2e0

Add missing imports

39ada92

Add secondary tryAcquire for testConfigure

2234363

Ignore Proxy test class

b9846f1

edaniels approved these changes Oct 22, 2018

View reviewed changes

adamchel suggested changes Oct 22, 2018

View reviewed changes

jsflax added 2 commits October 24, 2018 13:43

Addressed comments

fd02447

removed unnecessary wait

23e6f64

adamchel approved these changes Oct 24, 2018

View reviewed changes

jsflax merged commit 9da14c8 into mongodb:master Oct 24, 2018


		dataSynchronizer.deleteOneById(namespace, document1["_id"])

		`when`(coreRemoteMongoCollectionMock.deleteOne(any())).thenAnswer {

		@@ -0,0 +1,567 @@
		package com.mongodb.stitch.core.services.mongodb.remote.sync.internal

STITCH-1956 Fill in missing unit tests for every relevant task and clean exisiting tests. #69

STITCH-1956 Fill in missing unit tests for every relevant task and clean exisiting tests. #69

Uh oh!

Conversation

jsflax commented Oct 12, 2018

Uh oh!

coveralls commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edaniels 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!

adamchel 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!

jsflax commented Oct 22, 2018

Uh oh!

edaniels 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!

adamchel 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

coveralls commented Oct 12, 2018 •

edited

Loading