CBL-3341: Review and Complete Collection Tests #101

LaurenNguyen14 · 2022-07-11T18:12:44Z

CBL-3357: Validate 32+ select items in the query fails or not
Fix the error code for getting documents from a deleted collection

CBL-3357: Validate 32+ select items in the query fails or not

bmeike

This looks great.

I've left some comments but the most important one is that I'd like you to use the name randomizer whenever you create something that might conflict with the next test. We had a lot of trouble, in the past, with one test failing and causing a cascade of other tests to fail, because the failing test left cruft around.

bmeike · 2022-07-11T18:23:36Z

common/main/java/com/couchbase/lite/AbstractDatabase.java

 @Deprecated
 @Nullable
 public Document getDocument(@NonNull String id) {
 try { return getDefaultCollectionOrThrow().getDocument(id); }
- catch (CouchbaseLiteException e) { throw new IllegalStateException(Log.lookupStandardMessage("DBClosed"), e); }
+ catch (CouchbaseLiteException e) {
+ if (e.getCode() == CBLError.Code.NOT_OPEN) {


Yeah.... nice. You might have to check the Domain, too...

bmeike · 2022-07-11T18:24:22Z

common/main/java/com/couchbase/lite/Collection.java

- return null;
+ catch (CouchbaseLiteException e) {
+ if (e.getCode() == CBLError.Code.NOT_FOUND) {
+ Log.i(LogDomain.DATABASE, "Failed retrieving document: %s", id);


I don't think you need to log this. It will happen all the time...

... and, again, might want to check the domain

bmeike · 2022-07-11T18:27:16Z

common/test/java/com/couchbase/lite/BaseCollectionTest.kt


 protected val Scope.collectionCount
 get() = this.collections.size

 @Before
 fun setUpBaseCollectionTest() {
- testScope = baseTestDb.defaultScope
- testCollection = testScope!!.getCollection(Collection.DEFAULT_NAME)
+ testColName = "test_collection"


Consider using the name randomizer: BaseTest.getUniqueName

common/test/java/com/couchbase/lite/BaseDbTest.java

bmeike · 2022-07-11T18:36:08Z

common/test/java/com/couchbase/lite/BaseDbTest.java

@@ -1655,14 +1665,38 @@ protected final void verifyDocument(DictionaryInterface doc) {
 verifyBlob(doc.getBlob("doc-29"));
 }

+ protected Database openDatabase() throws CouchbaseLiteException { return verifyDb(createDb("test_db")); }


Please use the name randomizer, here. We've had a a lot of trouble, in the past, with tests failing because of some cruft left around by previous tests.

bmeike · 2022-07-11T18:45:52Z

common/test/java/com/couchbase/lite/BaseCollectionTest.kt

+ for (i in 0 until n) {
+ val doc = MutableDocument(String.format(Locale.US, "doc_%03d", i))
+ doc.setValue("key", i)
+ saveDocInBaseCollectionTest(doc)


Is the "Base" collection the default collection?

No, it's the test collection that is created in BaseCollectionTest, but I'll change the name of this function to avoid confusion

I see. Cool!

bmeike · 2022-07-11T18:49:24Z

common/test/java/com/couchbase/lite/CollectionTest.kt

+ val docID = "doc1"
+ val doc = MutableDocument(docID)
+
+ val coll1 = baseTestDb.createCollection("coll_1")


I do worry that many of these tests will fail, if one of them does. We've had that happen a lot in the past. I really do suggest using the name randomizer.

common/test/java/com/couchbase/lite/CollectionTest.kt

bmeike · 2022-07-11T18:51:42Z

common/test/java/com/couchbase/lite/CollectionTest.kt

+
+ try {
+ assertNotSame(collection, testCollection)
+ if (collection != null) {


should you assert not null here?

bmeike · 2022-07-11T18:52:14Z

common/test/java/com/couchbase/lite/CollectionTest.kt

+
+ // Update doc and store it into different instance
+ doc.setValue("key", 2)
+ TestUtils.assertThrowsCBL(CBLError.Domain.CBLITE, CBLError.Code.INVALID_PARAMETER) {


bmeike

Add the name randomizer and see what you think of the other comments. Go ahead and commit.

bmeike · 2022-07-12T00:37:15Z

common/main/java/com/couchbase/lite/Collection.java

- catch (CouchbaseLiteException e) { Log.i(LogDomain.DATABASE, "Failed retrieving document: %s", id); }
- return null;
+ catch (CouchbaseLiteException e) {
+ if (e.getDomain().equals(CBLError.Domain.CBLITE) && e.getCode() == CBLError.Code.NOT_FOUND) {


not requesting a change, here, but FYI:
CBLError.Domain.CBLITE will never be null. That means that saying CBLError.Domain.CBLITE.equals(...) will never NPE.

common/test/java/com/couchbase/lite/BaseCollectionTest.kt

bmeike · 2022-07-12T00:39:44Z

common/test/java/com/couchbase/lite/BaseCollectionTest.kt

+ for (i in 0 until n) {
+ val doc = MutableDocument(String.format(Locale.US, "doc_%03d", i))
+ doc.setValue("key", i)
+ saveDocInBaseCollectionTest(doc)


I see. Cool!

bmeike · 2022-07-12T00:40:19Z

common/test/java/com/couchbase/lite/BaseDbTest.java

@@ -55,6 +59,10 @@ public interface DocValidator extends Fn.ConsumerThrows<Document, CouchbaseLiteE

 public static C4Database getC4Db(@NonNull Database db) { return db.getOpenC4Database(); }

+ protected static <T extends Comparable<T>> void assertContents(List<T> l1, T... contents) {


Oh!! Very nice!

bmeike · 2022-07-12T00:42:38Z

common/test/java/com/couchbase/lite/BaseDbTest.java

+ deleteDb(db);
+ throw new AssertionError("Unable to get db path", e);
+ }
+ catch (AssertionError e) {


Ahhh!!!! Gottcha. This isn't a test, its a utility method.
In that case, I don't think it should delete the database at all. It should return true or false and the db creator should be responsible for doing the right thing...

bmeike · 2022-07-12T00:43:26Z

common/test/java/com/couchbase/lite/BaseDbTest.java

+ deleteDb(db);
+ throw new AssertionError("Unable to get db path", e);
+ }
+ catch (AssertionError e) {


Or.... if that would break a lot of legacy code, just rename it "verifyOrDeleteDb"...

bmeike

I haven't looked at all of the tests... that, of course, is the important part. Please do walk through them and assure yourself that they are actually testing what they are meant to test.

... and I ask you to do this before, so if you've already done it, just commit. It looks really good.

common/test/java/com/couchbase/lite/BaseCollectionTest.kt

CBL-3341: Review and Complete Collection Tests

5917c48

CBL-3357: Validate 32+ select items in the query fails or not

LaurenNguyen14 requested a review from bmeike July 11, 2022 18:12

bmeike reviewed Jul 11, 2022

View reviewed changes

bmeike requested changes Jul 11, 2022

View reviewed changes

Address PR feedback

862fd94

LaurenNguyen14 requested a review from bmeike July 11, 2022 23:36

fix DBCollectionsTest

f0e2361

bmeike approved these changes Jul 12, 2022

View reviewed changes

Refactor

1b6dddc

LaurenNguyen14 merged commit 1da811c into master Jul 12, 2022

LaurenNguyen14 deleted the fix/CBL-3341 branch July 12, 2022 17:32

bmeike reviewed Jul 12, 2022

View reviewed changes

common/test/java/com/couchbase/lite/BaseCollectionTest.kt Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBL-3341: Review and Complete Collection Tests #101

CBL-3341: Review and Complete Collection Tests #101

LaurenNguyen14 commented Jul 11, 2022

bmeike left a comment

bmeike Jul 11, 2022

bmeike Jul 11, 2022

bmeike Jul 11, 2022

bmeike Jul 11, 2022

bmeike Jul 11, 2022

LaurenNguyen14 Jul 11, 2022

bmeike Jul 12, 2022

bmeike Jul 11, 2022

bmeike Jul 11, 2022

bmeike Jul 11, 2022

bmeike left a comment

bmeike Jul 12, 2022

bmeike Jul 12, 2022

bmeike Jul 12, 2022

bmeike Jul 12, 2022

bmeike Jul 12, 2022

bmeike left a comment

		@@ -55,6 +59,10 @@ public interface DocValidator extends Fn.ConsumerThrows<Document, CouchbaseLiteE

		public static C4Database getC4Db(@NonNull Database db) { return db.getOpenC4Database(); }

		protected static <T extends Comparable<T>> void assertContents(List<T> l1, T... contents) {

CBL-3341: Review and Complete Collection Tests #101

CBL-3341: Review and Complete Collection Tests #101

Conversation

LaurenNguyen14 commented Jul 11, 2022

bmeike 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeike 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

bmeike left a comment

Choose a reason for hiding this comment