CSHARP-4420: EG tests for Atlas Search #1001

BorisDog · 2022-12-22T18:33:31Z

This PR is rebased on top of CSHARP-4440.
The main focus here is AtlasSearchTest.cs
Please review just the last commit.

This reverts commit 77d144c.

…Set method.

…edException.

…ongodb#935)

…guments are not null.

…t LINQ3 instead of or in addition to LINQ2.

…#961)

DmitryLukyanov

Some mostly minor comments

DmitryLukyanov · 2023-01-06T11:48:17Z

src/MongoDB.Driver/Search/Highlight.cs

@@ -0,0 +1,79 @@
+// Copyright 2010-present MongoDB Inc.


copyright has a bit different commenting format, see other files

Good catch, I'll fix this in the main search PR too.

not sure whether it's already should be fixed, but if so - not fixed yet

for the future :( I would suggest using a different PR approach (we already did similar work in past) since it's hard to follow what was fixed and what should be reviewed. Sounds like at least in this file the above suggestion was applied..

DmitryLukyanov · 2023-01-06T11:49:21Z

tests/MongoDB.Driver.Tests/Search/AtlasSearchTest.cs

+    [Trait("Category", "AtlasSearch")]
+    public class AtlasSearchTest : LoggableTestClass
+    {
+        private static readonly GeoJsonPolygon<GeoJson2DGeographicCoordinates> __testPolygon =


put static fields into region static

DmitryLukyanov · 2023-01-06T11:50:04Z

tests/MongoDB.Driver.Tests/Search/AtlasSearchTest.cs

+    public class AtlasSearchTest : LoggableTestClass
+    {
+        private static readonly GeoJsonPolygon<GeoJson2DGeographicCoordinates> __testPolygon =
+            new(new(new(new GeoJson2DGeographicCoordinates[]


I have no ideas what happens here =)) Can you explain?

This is a long chain of classes creation with long names shortened to omit the names for readability.
So for example in creation of GeoWithinBox, the only important and visible data is its 2 corners.

I didn't know about this feature. Sounds like it automatically recognizes which type is expected to be created and which arguments expected to be passed into. It looks "cute", but only until you really don't care about what actual types are needed to create the target type. Currently it might be true, next time it might be changed. It can be problematic especially if you're looking at the code from github.. I probably don't mind about it especially in a test like this, but I would use it carefully.

omit the names for readability

LOL.

I found it much harder to read because there is no way to know what each call to new is doing without going into Visual Studio and hovering the mouse over each new to see what it does.

I agree with Dima. This looks like an abuse of a new C# feature.

DmitryLukyanov · 2023-01-06T11:50:51Z

tests/MongoDB.Driver.Tests/Search/AtlasSearchTest.cs

+            _disposableMongoClient.Dispose();
+
+        [Fact]
+        public void Autocomplete()


Let's use more test related naming. At the very least: Autocomplete_test

I am not big fun of having same prefix for all test methods, especially when it does not add any information. Can you think of other alternatives here?

at the very least it will allow to determine where actual method and where a test for this method in VS UI. Also you may look at best practice for testing here (search for Naming your tests). As for example of a better name, it can be: Autocomplete should produce expected outcome(or how it's called?)

DmitryLukyanov · 2023-01-06T11:58:26Z

tests/MongoDB.Driver.Tests/Search/AtlasSearchTest.cs

+        [Fact]
+        public void MustNot()
+        {
+            var result = SearchSingle(Builders.Search.Compound()


I would recommend better formatting here (and in other places), something like:

var result = SearchSingle( Builders.Search.Compound().MustNot( Builders.Search.Phrase("life, liberty", x => x.Body))); result.Title.Should().Be("US Constitution");

or maybe better creating new variables for inner steps

DmitryLukyanov · 2023-01-06T11:59:56Z

evergreen/run-atlas-search-test.sh

+
+export ATLAS_SEARCH_TESTS_ENABLED=true
+
+powershell.exe .\\build.ps1 --target TestAtlasSearch


do we have plans for non windows envs?

Choice for Windows platform is arbitrary, but we don't plan to extend to other OS.

got it, I wrote it for 2 reasons:

Better to use target settings approach like: --target=$TARGET that works on both windows and non windows machines

powershell is only windows tool, so given you don't have validation on OS, potentially it might throw, but it's minor, see whether you want to change anything here

I think it's fine for now, just copied from run-atlas-data-lake-test and run-atlas-connectivity-tests

James told me once that we had to use --target=TestAtlasSearch.

I.e. that the = was mandatory.

it's mandatory only on non windows OSs if I recall correctly

At one point it was mandatory on my Windows machine also, so I got used to using the = form.

My guess is that at some point a Cake release made it mandatory, and they got lots of complaints, and then they reverted the change.

I'm assuming it's Cake that parses this argument(s) and not Powershell.

DmitryLukyanov · 2023-01-06T12:01:16Z

evergreen/run-atlas-search-test.sh

+#!/usr/bin/env bash
+
+set -o xtrace
+set -o errexit  # Exit the script with error if any of the commands fail


add a section like in most of other files about output variables: https://github.com/mongodb/mongo-csharp-driver/blob/master/evergreen/run-csfle-azure-tests.sh#L11

DmitryLukyanov · 2023-01-06T12:02:09Z

src/MongoDB.Driver/Search/Highlight.cs

+        /// Gets or sets the document field which returned a match.
+        /// </summary>
+        [BsonElement("path")]
+        public string Path { get; private set; }


why private set is here? Sounds like it does nothing?

sounds like it's for deserialization only. Does it deserve a comment?

Yeah it's for deserialization. Looks pretty minor for a comment.

DmitryLukyanov · 2023-01-06T12:03:24Z

tests/MongoDB.Driver.Tests/Search/AtlasSearchTest.cs

+            _disposableMongoClient = new(new MongoClient(atlasSearchUri), CreateLogger<DisposableMongoClient>());
+        }
+
+        protected override void DisposeInternal() =>


nit: I think all content suites in one line like:
protected override void DisposeInternal() => _disposableMongoClient.Dispose();

DmitryLukyanov · 2023-01-06T12:06:11Z

tests/MongoDB.Driver.Tests/Search/AtlasSearchTest.cs

+            GetGeoTestCollection().Aggregate().Search(searchDefintion).ToList();
+
+        private HistoricalDocument SearchSingle(SearchDefinition<HistoricalDocument> searchDefintion) =>
+            GetTestCollection().Aggregate().Search(searchDefintion)


this should be at least:

GetTestCollection().Aggregate().Search(searchDefintion) .Limit(1) .ToList() .Single();

but I would recommend a better formatting:

GetTestCollection() .Aggregate() .Search(searchDefintion) .Limit(1) .ToList() .Single();

DmitryLukyanov

some mostly minor comments

DmitryLukyanov · 2023-01-07T23:32:37Z

tests/MongoDB.Driver.Tests/Search/AtlasSearchTest.cs

+            _disposableMongoClient.Dispose();
+
+        [Fact]
+        public void Autocomplete()


at the very least it will allow to determine where actual method and where a test for this method in VS UI. Also you may look at best practice for testing here (search for Naming your tests). As for example of a better name, it can be: Autocomplete should produce expected outcome(or how it's called?)

DmitryLukyanov · 2023-01-09T14:25:34Z

evergreen/run-atlas-search-test.sh

+
+export ATLAS_SEARCH_TESTS_ENABLED=true
+
+powershell.exe .\\build.ps1 --target TestAtlasSearch


got it, I wrote it for 2 reasons:

Better to use target settings approach like: --target=$TARGET that works on both windows and non windows machines

powershell is only windows tool, so given you don't have validation on OS, potentially it might throw, but it's minor, see whether you want to change anything here

DmitryLukyanov · 2023-01-09T14:26:20Z

src/MongoDB.Driver/Search/Highlight.cs

@@ -0,0 +1,79 @@
+// Copyright 2010-present MongoDB Inc.


not sure whether it's already should be fixed, but if so - not fixed yet

DmitryLukyanov · 2023-01-09T14:29:29Z

src/MongoDB.Driver/Search/Highlight.cs

@@ -0,0 +1,79 @@
+// Copyright 2010-present MongoDB Inc.


for the future :( I would suggest using a different PR approach (we already did similar work in past) since it's hard to follow what was fixed and what should be reviewed. Sounds like at least in this file the above suggestion was applied..

DmitryLukyanov · 2023-01-09T14:32:12Z

tests/MongoDB.Driver.Tests/Search/AtlasSearchTest.cs

+            var atlasSearchUri = Environment.GetEnvironmentVariable("ATLAS_SEARCH");
+            Ensure.IsNotNullOrEmpty(atlasSearchUri, nameof(atlasSearchUri));
+
+            _disposableMongoClient = new(new MongoClient(atlasSearchUri), CreateLogger<DisposableMongoClient>());


I would suggest creating a new CreateDisposableClient(connectionString) method to DriverTestConfiguration to be able creating a test client in the same way across driver tests

I would suggest doing this in different small refactoring PR, to change all other places too.

DmitryLukyanov · 2023-01-09T14:51:53Z

tests/MongoDB.Driver.Tests/Search/AtlasSearchTest.cs

+    public class AtlasSearchTest : LoggableTestClass
+    {
+        private static readonly GeoJsonPolygon<GeoJson2DGeographicCoordinates> __testPolygon =
+            new(new(new(new GeoJson2DGeographicCoordinates[]


I didn't know about this feature. Sounds like it automatically recognizes which type is expected to be created and which arguments expected to be passed into. It looks "cute", but only until you really don't care about what actual types are needed to create the target type. Currently it might be true, next time it might be changed. It can be problematic especially if you're looking at the code from github.. I probably don't mind about it especially in a test like this, but I would use it carefully.

DmitryLukyanov

LGTM

rstam

Minor changes requested. Overall looks good.

I think this needs to be reviewed again (in a new clean PR) once you have rebased this on the csharp4440 work.

rstam · 2023-01-17T17:21:46Z

evergreen/run-atlas-search-test.sh

+
+export ATLAS_SEARCH_TESTS_ENABLED=true
+
+powershell.exe .\\build.ps1 --target TestAtlasSearch


James told me once that we had to use --target=TestAtlasSearch.

I.e. that the = was mandatory.

rstam · 2023-01-17T17:24:05Z

src/MongoDB.Driver/Search/Highlight.cs

+    /// <summary>
+    /// Represents a result of highlighting.
+    /// </summary>
+    public sealed class Highlight


Why is this new file in this PR? Shouldn't all work that is not testing related be in the CSHARP-4440 PR?

In any case, I'm not reviewing this file in this PR because it's not testing related.

This whole folder is wildly out of date with CSHRP-4440.

rstam · 2023-01-17T17:34:57Z

tests/MongoDB.Driver.Tests/Search/AtlasSearchTest.cs

+namespace MongoDB.Driver.Tests.Search
+{
+    [Trait("Category", "AtlasSearch")]
+    public class AtlasSearchTest : LoggableTestClass


nit: rename file and class to AtlasSearchTests (plural)

rstam · 2023-01-17T17:37:43Z

tests/MongoDB.Driver.Tests/Search/AtlasSearchTest.cs

+    public class AtlasSearchTest : LoggableTestClass
+    {
+        private static readonly GeoJsonPolygon<GeoJson2DGeographicCoordinates> __testPolygon =
+            new(new(new(new GeoJson2DGeographicCoordinates[]


omit the names for readability

LOL.

I found it much harder to read because there is no way to know what each call to new is doing without going into Visual Studio and hovering the mouse over each new to see what it does.

I agree with Dima. This looks like an abuse of a new C# feature.

BorisDog and others added 20 commits December 2, 2022 10:04

- Search text operator

facf8a1

CSHARP-4440: Incorporate MongoDB.Labs.Search library

0c9e2a6

- Tests fix

7bcf7c3

Hide aws sdk package. (mongodb#981)

bd3bb51

Revert "Hide aws sdk package. (mongodb#981)" (mongodb#982)

332b09a

This reverts commit 77d144c.

CSHARP-4428: LINQ3 not handling down cast in UpdateDefinitionBuilder …

ad74cdf

…Set method.

CSHARP-4421: Unify all spec tests in single test project (mongodb#977)

ff4c396

CSHARP-4427: Negative array indexes should throw ExpressionNotSupport…

cae2582

…edException.

CSHARP-4399: Remove use of activate_venv.sh and utils.sh. (mongodb#983)

20bc1eb

CSHARP-4400: Upgrade xunit to 2.4.2 and remove SkippableFact package (m…

40b011d

…ongodb#935)

CSHARP-4446: MongoQueryable methods should validate that mandatory ar…

0daabd7

…guments are not null.

CSHARP-4339: Added database.AsQueryable().

4d81692

CSHARP-4339: Add support for $documents in LINQ.

43d8939

CSHARP-4358: Update tests that only test against LINQ2 to test agains…

5ce4903

…t LINQ3 instead of or in addition to LINQ2.

CSHARP-4445: Support 64-bit values for Skip and Limit/Take.

cc9a895

Fix compilation errors resulting from merge.

586b2cc

CSHARP-4255: Automatically create Queryable Encryption keys. (mongodb…

f159956

…#961)

CSHARP-4255: Fix merge conflict. (mongodb#992)

454f37c

CSHARP-4455: Fix test assertation. (mongodb#994)

b3ca622

CSHARP-4420: EG tests for Atlas Search

cb8a4be

BorisDog requested a review from JamesKovacs December 22, 2022 18:33

JamesKovacs requested review from DmitryLukyanov and removed request for JamesKovacs January 6, 2023 01:44

DmitryLukyanov reviewed Jan 6, 2023

View reviewed changes

- PR comments

150dd8a

BorisDog requested a review from DmitryLukyanov January 6, 2023 23:29

DmitryLukyanov reviewed Jan 9, 2023

View reviewed changes

BorisDog requested a review from DmitryLukyanov January 9, 2023 23:33

DmitryLukyanov approved these changes Jan 9, 2023

View reviewed changes

rstam self-requested a review January 12, 2023 15:38

rstam requested changes Jan 17, 2023

View reviewed changes

BorisDog closed this Jan 19, 2023

BorisDog deleted the csharp4420 branch January 19, 2023 16:20


		export ATLAS_SEARCH_TESTS_ENABLED=true

		powershell.exe .\\build.ps1 --target TestAtlasSearch

CSHARP-4420: EG tests for Atlas Search #1001

CSHARP-4420: EG tests for Atlas Search #1001

Conversation

BorisDog commented Dec 22, 2022 • edited

DmitryLukyanov 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

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

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

DmitryLukyanov 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

DmitryLukyanov left a comment

Choose a reason for hiding this comment

rstam 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

BorisDog commented Dec 22, 2022 •

edited