From 1e27da2abf03710f8b571a27f087ec7a68d446ee Mon Sep 17 00:00:00 2001 From: Jim Borden Date: Sat, 22 Jun 2024 11:21:37 +0900 Subject: [PATCH 1/5] Fix logic error in c4coll_isIndexTrained It was universally returning true. Also remove the warnings check from the VS test since it makes no sense anymore and is really hard to in the isTrained test now that "untrained" is a warning. Add in a test for the C API to make sure it works correctly. --- C/c4CAPI.cc | 2 +- LiteCore/tests/LiteCoreTest.cc | 16 +--------------- LiteCore/tests/LiteCoreTest.hh | 1 - LiteCore/tests/VectorQueryTest.cc | 22 ++++++++++++++++++++-- LiteCore/tests/VectorQueryTest.hh | 8 -------- 5 files changed, 22 insertions(+), 27 deletions(-) diff --git a/C/c4CAPI.cc b/C/c4CAPI.cc index 782d10164..defdfdb54 100644 --- a/C/c4CAPI.cc +++ b/C/c4CAPI.cc @@ -558,7 +558,7 @@ bool c4db_createIndex2(C4Database* database, C4Slice name, C4Slice indexSpec, C4 bool c4coll_isIndexTrained(C4Collection* collection, C4Slice name, C4Error* outError) noexcept { if ( outError ) *outError = kC4NoError; - return tryCatch(outError, [=] { return collection->isIndexTrained(name); }); + return tryCatch(outError, [=] { return collection->isIndexTrained(name); }); } // semi-deprecated diff --git a/LiteCore/tests/LiteCoreTest.cc b/LiteCore/tests/LiteCoreTest.cc index d13ecd14a..936ac9ca8 100644 --- a/LiteCore/tests/LiteCoreTest.cc +++ b/LiteCore/tests/LiteCoreTest.cc @@ -76,23 +76,11 @@ void ExpectException(litecore::error::Domain domain, int code, const std::functi #pragma mark - TESTFIXTURE: - -static LogDomain::Callback_t sPrevCallback; -static atomic_uint sWarningsLogged; - -static void logCallback(const LogDomain& domain, LogLevel level, const char* fmt, va_list args) { - if ( level >= LogLevel::Warning ) { ++sWarningsLogged; } - sPrevCallback(domain, level, fmt, args); -} - -TestFixture::TestFixture() : _warningsAlreadyLogged(sWarningsLogged), _objectCount(c4_getObjectCount()) { +TestFixture::TestFixture() : _objectCount(c4_getObjectCount()) { static once_flag once; call_once(once, [] { InitTestLogging(); - sPrevCallback = LogDomain::currentCallback(); - LogDomain::setCallback(&logCallback, false); - #if TARGET_OS_IPHONE // iOS tests copy the fixture files into the test bundle. CFBundleRef bundle = CFBundleGetBundleWithIdentifier(CFSTR("org.couchbase.LiteCoreTests")); @@ -119,8 +107,6 @@ TestFixture::~TestFixture() { } } -unsigned TestFixture::warningsLogged() const noexcept { return sWarningsLogged - _warningsAlreadyLogged; } - FilePath TestFixture::GetPath(const string& name, const string& extension) noexcept { static chrono::milliseconds unique; diff --git a/LiteCore/tests/LiteCoreTest.hh b/LiteCore/tests/LiteCoreTest.hh index 4be0cbaba..23ee1df72 100644 --- a/LiteCore/tests/LiteCoreTest.hh +++ b/LiteCore/tests/LiteCoreTest.hh @@ -100,7 +100,6 @@ class TestFixture { static FilePath sTempDir; private: - unsigned const _warningsAlreadyLogged; int _objectCount; }; diff --git a/LiteCore/tests/VectorQueryTest.cc b/LiteCore/tests/VectorQueryTest.cc index 50a5ce915..960beebd1 100644 --- a/LiteCore/tests/VectorQueryTest.cc +++ b/LiteCore/tests/VectorQueryTest.cc @@ -20,6 +20,7 @@ #include "Base64.hh" #include "c4Database.hh" #include "c4Collection.hh" +#include "c4Index.h" #include "c4Database.h" #ifdef COUCHBASE_ENTERPRISE @@ -221,7 +222,6 @@ N_WAY_TEST_CASE_METHOD(SIFTVectorQueryTest, "Query Vector Index", "[Query][.Vect enc.writeString("nope"); }); t.commit(); - ++expectedWarningsLogged; } // Verify the updated document is missing from the results: Query::Options options = optionsWithTargetVector(kTargetVector, kData); @@ -566,6 +566,7 @@ static pair splitCollectionName(const string& input) { // may change based on usability concerns. TEST_CASE_METHOD(SIFTVectorQueryTest, "Index isTrained API", "[Query][.VectorSearch]") { bool expectedTrained{false}; + bool expectedPretrained{false}; // Undo this silliness, I'm not spending the effort to find out the name it really wants // which is LiteCore_Tests_ or something @@ -586,6 +587,7 @@ TEST_CASE_METHOD(SIFTVectorQueryTest, "Index isTrained API", "[Query][.VectorSea } expectedTrained = false; + expectedPretrained = false; createVectorIndex(); readVectorDocs(100); } @@ -602,6 +604,7 @@ TEST_CASE_METHOD(SIFTVectorQueryTest, "Index isTrained API", "[Query][.VectorSea } expectedTrained = true; + expectedPretrained = true; createVectorIndex(); readVectorDocs(256 * 30); } @@ -618,6 +621,7 @@ TEST_CASE_METHOD(SIFTVectorQueryTest, "Index isTrained API", "[Query][.VectorSea } expectedTrained = true; + expectedPretrained = false; readVectorDocs(256 * 30); createVectorIndex(); } @@ -648,6 +652,15 @@ TEST_CASE_METHOD(SIFTVectorQueryTest, "Index isTrained API", "[Query][.VectorSea } catch ( error& e ) { CHECK(e == error::InvalidParameter); } } + bool isTrained = collection->isIndexTrained("vecIndex"_sl); + CHECK(isTrained == expectedPretrained); + + C4Error err; + isTrained = c4coll_isIndexTrained(collection, "vecIndex"_sl, &err); + CHECK(err.code == 0); + CHECK(err.domain == 0); + CHECK(isTrained == expectedPretrained); + // Need to run an arbitrary query to actually train the index string queryStr = R"(SELECT META().id, publisher FROM )"s + collectionName + R"( WHERE VECTOR_MATCH(vecIndex, $target, 5) )"; @@ -662,7 +675,12 @@ TEST_CASE_METHOD(SIFTVectorQueryTest, "Index isTrained API", "[Query][.VectorSea Query::Options options(enc.finish()); Retained e(query->createEnumerator(&options)); - bool isTrained = collection->isIndexTrained("vecIndex"_sl); + isTrained = collection->isIndexTrained("vecIndex"_sl); + CHECK(isTrained == expectedTrained); + + isTrained = c4coll_isIndexTrained(collection, "vecIndex"_sl, &err); + CHECK(err.code == 0); + CHECK(err.domain == 0); CHECK(isTrained == expectedTrained); } diff --git a/LiteCore/tests/VectorQueryTest.hh b/LiteCore/tests/VectorQueryTest.hh index 2f23b1cd5..2b3ef0747 100644 --- a/LiteCore/tests/VectorQueryTest.hh +++ b/LiteCore/tests/VectorQueryTest.hh @@ -29,11 +29,6 @@ class VectorQueryTest : public QueryTest { VectorQueryTest(int which) : QueryTest(which + initialize()) {} - ~VectorQueryTest() { - // Assert that the callback did not log unexpected warnings: - CHECK(warningsLogged() == expectedWarningsLogged); - } - void requireExtensionAvailable() { if ( sExtensionPath.empty() ) FAIL("You must setenv LiteCoreExtensionPath, to the directory containing the " @@ -106,8 +101,5 @@ class VectorQueryTest : public QueryTest { Catch::Matchers::WithinRel(expectedDist, 0.20f) || Catch::Matchers::WithinAbs(expectedDist, 400.0f)); } - /// Increment this if the test is expected to generate a warning. - unsigned expectedWarningsLogged = 0; - static inline string sExtensionPath; }; From 129d90d983c776dcfdff20cc6d32a283486e59e5 Mon Sep 17 00:00:00 2001 From: Jim Borden Date: Tue, 25 Jun 2024 09:03:57 +0900 Subject: [PATCH 2/5] Fix warningsLogged in a place only used outside of Windows --- LiteCore/tests/LiteCoreTest.hh | 1 - LiteCore/tests/QueryTest.cc | 3 --- 2 files changed, 4 deletions(-) diff --git a/LiteCore/tests/LiteCoreTest.hh b/LiteCore/tests/LiteCoreTest.hh index 23ee1df72..a0fb8f510 100644 --- a/LiteCore/tests/LiteCoreTest.hh +++ b/LiteCore/tests/LiteCoreTest.hh @@ -93,7 +93,6 @@ class TestFixture { TestFixture(); ~TestFixture(); - unsigned warningsLogged() const noexcept; static litecore::FilePath GetPath(const std::string& name, const std::string& extension) noexcept; static std::string sFixturesDir; diff --git a/LiteCore/tests/QueryTest.cc b/LiteCore/tests/QueryTest.cc index 25cd7c69e..8b9057107 100644 --- a/LiteCore/tests/QueryTest.cc +++ b/LiteCore/tests/QueryTest.cc @@ -2150,9 +2150,6 @@ N_WAY_TEST_CASE_METHOD(QueryTest, "Query finalized after db deleted", "[Query]") // Now free the query enum, which will free the sqlite_stmt, triggering a SQLite warning // callback about the database file being unlinked: e = nullptr; - - // Assert that the callback did not log a warning: - CHECK(warningsLogged() == 0); } #endif From a12622cc6847f559140b367c4755aedb037c3a9e Mon Sep 17 00:00:00 2001 From: Jim Borden Date: Tue, 25 Jun 2024 09:15:31 +0900 Subject: [PATCH 3/5] formatting.... --- LiteCore/tests/LiteCoreTest.hh | 2 +- LiteCore/tests/VectorQueryTest.cc | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/LiteCore/tests/LiteCoreTest.hh b/LiteCore/tests/LiteCoreTest.hh index a0fb8f510..365f8a7ea 100644 --- a/LiteCore/tests/LiteCoreTest.hh +++ b/LiteCore/tests/LiteCoreTest.hh @@ -99,7 +99,7 @@ class TestFixture { static FilePath sTempDir; private: - int _objectCount; + int _objectCount; }; class DataFileTestFixture diff --git a/LiteCore/tests/VectorQueryTest.cc b/LiteCore/tests/VectorQueryTest.cc index 960beebd1..a8c841a45 100644 --- a/LiteCore/tests/VectorQueryTest.cc +++ b/LiteCore/tests/VectorQueryTest.cc @@ -586,7 +586,7 @@ TEST_CASE_METHOD(SIFTVectorQueryTest, "Index isTrained API", "[Query][.VectorSea store = &db->getKeyStore(string(".") + collectionName); } - expectedTrained = false; + expectedTrained = false; expectedPretrained = false; createVectorIndex(); readVectorDocs(100); @@ -603,7 +603,7 @@ TEST_CASE_METHOD(SIFTVectorQueryTest, "Index isTrained API", "[Query][.VectorSea store = &db->getKeyStore(string(".") + collectionName); } - expectedTrained = true; + expectedTrained = true; expectedPretrained = true; createVectorIndex(); readVectorDocs(256 * 30); @@ -620,7 +620,7 @@ TEST_CASE_METHOD(SIFTVectorQueryTest, "Index isTrained API", "[Query][.VectorSea store = &db->getKeyStore(string(".") + collectionName); } - expectedTrained = true; + expectedTrained = true; expectedPretrained = false; readVectorDocs(256 * 30); createVectorIndex(); From b392c9db1e4617c6aca261c2e6dc2a91645708c9 Mon Sep 17 00:00:00 2001 From: Jim Borden Date: Wed, 26 Jun 2024 06:39:14 +0900 Subject: [PATCH 4/5] Revert removing the warnings check Maybe this test is not as bad as it seems with it? It seems to pass locally now. --- LiteCore/tests/LiteCoreTest.cc | 16 +++++++++++++++- LiteCore/tests/LiteCoreTest.hh | 4 +++- LiteCore/tests/VectorQueryTest.cc | 2 ++ LiteCore/tests/VectorQueryTest.hh | 8 ++++++++ 4 files changed, 28 insertions(+), 2 deletions(-) diff --git a/LiteCore/tests/LiteCoreTest.cc b/LiteCore/tests/LiteCoreTest.cc index 936ac9ca8..d13ecd14a 100644 --- a/LiteCore/tests/LiteCoreTest.cc +++ b/LiteCore/tests/LiteCoreTest.cc @@ -76,11 +76,23 @@ void ExpectException(litecore::error::Domain domain, int code, const std::functi #pragma mark - TESTFIXTURE: -TestFixture::TestFixture() : _objectCount(c4_getObjectCount()) { + +static LogDomain::Callback_t sPrevCallback; +static atomic_uint sWarningsLogged; + +static void logCallback(const LogDomain& domain, LogLevel level, const char* fmt, va_list args) { + if ( level >= LogLevel::Warning ) { ++sWarningsLogged; } + sPrevCallback(domain, level, fmt, args); +} + +TestFixture::TestFixture() : _warningsAlreadyLogged(sWarningsLogged), _objectCount(c4_getObjectCount()) { static once_flag once; call_once(once, [] { InitTestLogging(); + sPrevCallback = LogDomain::currentCallback(); + LogDomain::setCallback(&logCallback, false); + #if TARGET_OS_IPHONE // iOS tests copy the fixture files into the test bundle. CFBundleRef bundle = CFBundleGetBundleWithIdentifier(CFSTR("org.couchbase.LiteCoreTests")); @@ -107,6 +119,8 @@ TestFixture::~TestFixture() { } } +unsigned TestFixture::warningsLogged() const noexcept { return sWarningsLogged - _warningsAlreadyLogged; } + FilePath TestFixture::GetPath(const string& name, const string& extension) noexcept { static chrono::milliseconds unique; diff --git a/LiteCore/tests/LiteCoreTest.hh b/LiteCore/tests/LiteCoreTest.hh index 365f8a7ea..4be0cbaba 100644 --- a/LiteCore/tests/LiteCoreTest.hh +++ b/LiteCore/tests/LiteCoreTest.hh @@ -93,13 +93,15 @@ class TestFixture { TestFixture(); ~TestFixture(); + unsigned warningsLogged() const noexcept; static litecore::FilePath GetPath(const std::string& name, const std::string& extension) noexcept; static std::string sFixturesDir; static FilePath sTempDir; private: - int _objectCount; + unsigned const _warningsAlreadyLogged; + int _objectCount; }; class DataFileTestFixture diff --git a/LiteCore/tests/VectorQueryTest.cc b/LiteCore/tests/VectorQueryTest.cc index a8c841a45..03edde9e0 100644 --- a/LiteCore/tests/VectorQueryTest.cc +++ b/LiteCore/tests/VectorQueryTest.cc @@ -222,6 +222,7 @@ N_WAY_TEST_CASE_METHOD(SIFTVectorQueryTest, "Query Vector Index", "[Query][.Vect enc.writeString("nope"); }); t.commit(); + ++expectedWarningsLogged; } // Verify the updated document is missing from the results: Query::Options options = optionsWithTargetVector(kTargetVector, kData); @@ -576,6 +577,7 @@ TEST_CASE_METHOD(SIFTVectorQueryTest, "Index isTrained API", "[Query][.VectorSea // extra collections here SECTION("Insufficient docs") { + ++expectedWarningsLogged; SECTION("As-is") {} SECTION("Default scope") { collectionName = "Secondary"; diff --git a/LiteCore/tests/VectorQueryTest.hh b/LiteCore/tests/VectorQueryTest.hh index 2b3ef0747..2f23b1cd5 100644 --- a/LiteCore/tests/VectorQueryTest.hh +++ b/LiteCore/tests/VectorQueryTest.hh @@ -29,6 +29,11 @@ class VectorQueryTest : public QueryTest { VectorQueryTest(int which) : QueryTest(which + initialize()) {} + ~VectorQueryTest() { + // Assert that the callback did not log unexpected warnings: + CHECK(warningsLogged() == expectedWarningsLogged); + } + void requireExtensionAvailable() { if ( sExtensionPath.empty() ) FAIL("You must setenv LiteCoreExtensionPath, to the directory containing the " @@ -101,5 +106,8 @@ class VectorQueryTest : public QueryTest { Catch::Matchers::WithinRel(expectedDist, 0.20f) || Catch::Matchers::WithinAbs(expectedDist, 400.0f)); } + /// Increment this if the test is expected to generate a warning. + unsigned expectedWarningsLogged = 0; + static inline string sExtensionPath; }; From 1c9ccccb6a9740f94b01b674bd8ead28eac4f6e8 Mon Sep 17 00:00:00 2001 From: Jim Borden Date: Wed, 26 Jun 2024 07:10:23 +0900 Subject: [PATCH 5/5] Forgot to restore one place --- LiteCore/tests/QueryTest.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/LiteCore/tests/QueryTest.cc b/LiteCore/tests/QueryTest.cc index 8b9057107..25cd7c69e 100644 --- a/LiteCore/tests/QueryTest.cc +++ b/LiteCore/tests/QueryTest.cc @@ -2150,6 +2150,9 @@ N_WAY_TEST_CASE_METHOD(QueryTest, "Query finalized after db deleted", "[Query]") // Now free the query enum, which will free the sqlite_stmt, triggering a SQLite warning // callback about the database file being unlinked: e = nullptr; + + // Assert that the callback did not log a warning: + CHECK(warningsLogged() == 0); } #endif