JAVA-5771 Add search integration tests #1616

katcharov · 2025-01-28T22:33:14Z

This adds AggregatesSearchTest, containing integration tests for the $search stage API.

Other files are simple refactoring (moving the atlas tests together in evg, renaming tasks and functions having the same name, etc.), unless otherwise noted.

katcharov · 2025-01-28T22:36:46Z

.evergreen/run-atlas-search-tests.sh

-    driver-core:test --tests AggregatesSearchIntegrationTest --tests AggregatesVectorSearchIntegrationTest
+    driver-core:test --tests AggregatesSearchIntegrationTest \
+    --tests AggregatesBinaryVectorSearchIntegrationTest \
+    --tests AggregatesSearchTest \


Fixes the missing Binary in AggregatesBinaryVectorSearchIntegrationTest. The tests were not previously being run.

katcharov · 2025-01-28T22:38:29Z

driver-core/src/main/com/mongodb/client/model/search/SearchOperator.java

     * @return The requested {@link SearchOperator}.
     * @mongodb.atlas.manual atlas-search/wildcard/ wildcard operator
     */
-    static WildcardSearchOperator wildcard(final String query, final SearchPath path) {
+    static WildcardSearchOperator wildcard(final SearchPath path, final String query) {


Parameter order is inconsistent. I refactored this (in a separate commit) to match at least regex, but this is already inconsistent in the API. I left a comment on the JIRA for revisiting the API.

katcharov · 2025-01-28T23:16:47Z

driver-sync/src/test/functional/com/mongodb/client/DatabaseTestCase.java

    protected CollectionHelper<Document> getCollectionHelper() {
-        return new CollectionHelper<>(new DocumentCodec(), getNamespace());
-    }


Above removals are old (several years) dead code.

katcharov · 2025-01-28T23:17:23Z

driver-sync/src/test/functional/com/mongodb/client/Fixture.java

@@ -59,34 +71,15 @@ public static synchronized MongoDatabase getDefaultDatabase() {
    }

    public static String getDefaultDatabaseName() {
-        return DEFAULT_DATABASE_NAME;


This has the same value as ClusterFixture

katcharov · 2025-01-28T23:19:49Z

driver-sync/src/test/functional/com/mongodb/client/Fixture.java

+                    return;
+                }
+                if (defaultDatabase != null) {
+                    defaultDatabase.drop();


This refactoring just moves the logic close to where it is used.

I do not really understand why we use a shutdownHook here, when elsewhere we rely on @AfterEach/All to clear resources. Perhaps we should either be using something that registers resources for shutdown with a shutdown hook, or just not using a shutdown hook at all.

katcharov · 2025-01-28T23:21:35Z

driver-sync/src/test/functional/com/mongodb/client/model/mql/AggregatesSearchTest.java

+                .findAny().orElse(null);
+    }
+
+    static void waitForIndex(final MongoCollection<Document> collection, final String indexName) {


This does duplicate functionality from the other test - it copies what I used in LangChain4j, and handles FAILED. I did not merge both, because that would require moving the other test to sync. I do not mind doing this, if that seems best.

vbabanin · 2025-02-04T01:52:33Z

.evergreen/.evg.yml


 - name: atlas-search-test
  display_name: "Atlas Search test"
  run_on: rhel80-small
  tasks:
    - name: "atlas-search-test"

+- name: atlas-search-variant
+  display_name: "Atlas Search Tests"


With the renaming "Atlas Search Index Management Tests" to "Atlas Search Tests", we will have two variants with the same name Atlas Search test and theseAtlas Search Tests. The only way to differentiate them would be by checking the tasks in Evergreen.

To make the distinction clearer at a glance, i think we should use more explicit naming. I suggest either keeping the previous name or renaming them as:

Atlas Search Test [Static Dev Cluster] – for the test using a pre-existing, always-running cluster.
Atlas Search Tests [Dynamic Cluster] – for the test that provisions a cluster, runs tests, and shuts it down.

I have merged these into one "Atlas Tests" variant, and have cleaned up related evg config. The data lake test remains on ubuntu2004-small; IIRC this is to ensure docker is available.

There is a failure in test-aws-lambda-deployed, however this predates this PR. When I ran it on 0e256548e6, see patch, it failed, though it passed on that commit previously - looks like an environment change.

vbabanin · 2025-02-04T03:14:23Z

driver-sync/src/test/functional/com/mongodb/client/model/mql/AggregatesSearchTest.java

+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
+
+public class AggregatesSearchTest {


Let’s use the FunctionalTest or IntegrationTest suffix here. This makes it easier to distinguish tests by their nature when searching or troubleshooting failures in Evergreen. We don't have particular convention but it seems that test classes without any suffix other than Test are usually unit tests. This also seems to align with the pattern that has been followed lately.

Also, should we make it abstract and share between sync and async for better coverage? What do you think?

I have renamed to FunctionalTest, but I am unsure - the root dir is called functional test. It seems that we could do a better job with naming and organizing our tests - the com.mongodb.client directory has many unorganized tests, and there are various inconsistencies with test naming (I clicked around and opened AbstractClientSideEncryptionDeadlockTest, and it is not a unit test).

As an aside, I think we should prefer not creating abstract classes, but instead write a single sync class, which we extend, with just the one getMongoClient override.

In this case, I do not think we are testing anything specific to the async side (that is, builders cannot possibly produce different results based on whether they are used from sync or async). Since the test is a usage test, I wonder if we should write it (and many other tests) using the actual async mongo client (instead of the wrapper), since the point of writing this in driver-sync rather than core is to see the interface "in context"? I am not sure about undertaking that in this PR, though.

I agree - we should establish a more structured and consistent approach to organizing tests. This is something worth deciding on.

Regarding avoiding abstract test classes, that makes sense to me. In practice, we already follow a similar pattern by overriding sync components with blocking async implementations.

As for the last point, I agree - this PR might not be the best place to make that change.

vbabanin · 2025-02-04T03:30:26Z

driver-sync/src/test/functional/com/mongodb/client/model/mql/AggregatesSearchTest.java

+    }
+
+    @Nullable
+    private static Document indexRecord(


i initially thought this method also indexes some record. I suggest we rename to find...():

Suggested change

private static Document indexRecord(

private static Document findSearchIndex(

vbabanin · 2025-02-04T03:37:29Z

driver-sync/src/test/functional/com/mongodb/client/model/mql/AggregatesSearchTest.java

+
+    static void waitForIndex(final MongoCollection<Document> collection, final String indexName) {
+        long startTime = System.nanoTime();
+        long timeoutNanos = TimeUnit.SECONDS.toNanos(20);


Should we use around around a minute here? I remember that it was taking more then 40 seconds sometimes for index to be created when BinaryVectorSearchIntegrationTests was executed.

Let's try with 20, and if it is failing, we can report the issue and update. This is supposed to be a near-empty database.

See also https://jira.mongodb.org/browse/CLOUDP-271832

vbabanin · 2025-02-04T03:41:47Z

driver-sync/src/test/functional/com/mongodb/client/model/mql/AggregatesSearchTest.java

+ * limitations under the License.
+ */
+
+package com.mongodb.client.model.mql;


I'm not sure if this package is the best fit for Atlas Search tests, as it seems to contain tests for the MQL API. Should we consider using the com.mongodb.client.model.search package, similar to driver-core?

vbabanin · 2025-02-04T03:45:08Z

...ore/src/test/functional/com/mongodb/client/model/search/AggregatesSearchIntegrationTest.java

@@ -121,6 +121,9 @@
 import static org.junit.jupiter.params.provider.Arguments.arguments;

 /**
+ * Use this class when needing to test against MFLIX specifically. Otherwise,


[nit] It might be more noticeable if placed right above public class AggregatesSearchIntegrationTest.

# Conflicts: # driver-core/src/main/com/mongodb/client/model/search/SearchOperator.java

vbabanin

LGTM!

katcharov · 2025-02-13T13:52:44Z

Current system failure, though renamed, has a base status of system failure, see example. Merging.

katcharov added 2 commits January 28, 2025 10:04

Add search integration tests

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

a6e18f9

Automated refactor fieldpath order

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

Loading
Loading status checks…

ec4a8e4

katcharov requested a review from vbabanin January 28, 2025 22:33

katcharov commented Jan 28, 2025

View reviewed changes

Add phrase and queryString tests

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

49c80c1

vbabanin reviewed Feb 4, 2025

View reviewed changes

katcharov added 2 commits February 4, 2025 12:23

Clean up evg

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

Loading
Loading status checks…

24d7d1e

Merge remote-tracking branch 'stIncMale/main' into JAVA-5771

Loading
Loading status checks…

b6d89a0

# Conflicts: # driver-core/src/main/com/mongodb/client/model/search/SearchOperator.java

katcharov requested a review from vbabanin February 4, 2025 22:25

Update waitForIndex

Loading
Loading status checks…

facee04

katcharov changed the title ~~Add search integration tests~~ JAVA-5771 Add search integration tests Feb 5, 2025

vbabanin approved these changes Feb 12, 2025

View reviewed changes

katcharov merged commit 36448cd into main Feb 13, 2025
60 of 62 checks passed

katcharov deleted the JAVA-5771 branch February 13, 2025 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JAVA-5771 Add search integration tests #1616

JAVA-5771 Add search integration tests #1616

katcharov commented Jan 28, 2025

katcharov Jan 28, 2025

katcharov Jan 28, 2025

katcharov Jan 28, 2025

katcharov Jan 28, 2025

katcharov Jan 28, 2025

katcharov Jan 28, 2025

vbabanin Feb 4, 2025

katcharov Feb 4, 2025

vbabanin Feb 4, 2025

katcharov Feb 4, 2025

vbabanin Feb 8, 2025

vbabanin Feb 4, 2025

vbabanin Feb 4, 2025

katcharov Feb 4, 2025

vbabanin Feb 4, 2025

vbabanin Feb 4, 2025 •

edited

Loading

vbabanin left a comment

katcharov commented Feb 13, 2025

	private static Document indexRecord(
	private static Document findSearchIndex(

JAVA-5771 Add search integration tests #1616

JAVA-5771 Add search integration tests #1616

Conversation

katcharov commented Jan 28, 2025

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

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

vbabanin Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

vbabanin left a comment

Choose a reason for hiding this comment

katcharov commented Feb 13, 2025

vbabanin Feb 4, 2025 •

edited

Loading