CSHARP-3705: Ignore hello commands during event capture for UnifiedTestRunner. #548

JamesKovacs · 2021-06-10T21:07:24Z

Running patch on latest to verify the fix:
https://spruce.mongodb.com/version/60c27902d1fe0735af1ee280/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

…stRunner.

jyemin

LGTM once test runs confirm it.

…tests related to not-yet-implemented load balancer support.

JamesKovacs · 2021-06-11T20:07:13Z

The initial patch revealed additional failures. All fixes are in the test runner code, not production code. These fixes ran cleanly on a 5.0-rc0 replset with and without auth locally. Running full patch on latest in Evergreen:

https://spruce.mongodb.com/version/60c3c19c7742ae54c1a12358/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

That should tell us if I caught everything this time.

… output from 5.0 sharded clusters.

JamesKovacs · 2021-06-11T23:55:27Z

Evergreen found failures on 5.0 sharded clusters because the query team changed the explain plan format for sharded clusters. Some of our tests were looking for explain plan output in the wrong place and failing with a null reference on 5.0 sharded clusters only. Let's see if this patch reports green...

https://spruce.mongodb.com/version/60c3ef88d1fe0769f894b6f0/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

DmitryLukyanov

Should we make anything with isMasterResult also? like wrap it to HelloResult and use a new class in the code instead. Will we be able just to remove isMasterResult in 3.0 without depricating this class now?

DmitryLukyanov · 2021-06-12T16:03:42Z

tests/MongoDB.Driver.Core.TestHelpers/XunitExtensions/RequireServer.cs

@@ -222,6 +222,11 @@ private bool CanRunOn(ICluster cluster, BsonDocument requirement)
            {
                switch (item.Name)
                {
+                    case "authEnabled":


should it be just auth like here: https://github.com/mongodb/specifications/blob/master/source/load-balancers/tests/sdam-error-handling.json#L324?

authEnabled is a new field being added with the updated SDAM spec tests.

oh, it's for non-unified format, it would be good if when we add a new logic to both old and new formats, we tried to choose similar keys :)

DmitryLukyanov · 2021-06-12T16:09:41Z

...sts/Specifications/server-discovery-and-monitoring/ServerDiscoveryAndMonitoringTestRunner.cs

@@ -529,7 +530,8 @@ private ICluster BuildCluster(BsonDocument definition)
        private class TestCaseFactory : JsonDrivenTestCaseFactory
        {
            // private constants
-            private const string MonitoringPrefix = "MongoDB.Driver.Core.Tests.Specifications.server_discovery_and_monitoring.tests.monitoring.";
+            private readonly string[] MonitoringPrefixes = {"MongoDB.Driver.Core.Tests.Specifications.server_discovery_and_monitoring.tests.monitoring.",


nit:

private readonly string[] MonitoringPrefixes = { "MongoDB.Driver.Core.Tests.Specifications.server_discovery_and_monitoring.tests.monitoring.", "MongoDB.Driver.Core.Tests.Specifications.server_discovery_and_monitoring.tests.legacy_hello.monitoring." };

but in any case we use a space between { and inner content

DmitryLukyanov · 2021-06-12T16:13:29Z

...sts/Specifications/server-discovery-and-monitoring/ServerDiscoveryAndMonitoringTestRunner.cs

@@ -529,7 +530,8 @@ private ICluster BuildCluster(BsonDocument definition)
        private class TestCaseFactory : JsonDrivenTestCaseFactory
        {
            // private constants
-            private const string MonitoringPrefix = "MongoDB.Driver.Core.Tests.Specifications.server_discovery_and_monitoring.tests.monitoring.";
+            private readonly string[] MonitoringPrefixes = {"MongoDB.Driver.Core.Tests.Specifications.server_discovery_and_monitoring.tests.monitoring.",
+                                                            "MongoDB.Driver.Core.Tests.Specifications.server_discovery_and_monitoring.tests.legacy_hello.monitoring."};


where is this line from, i don't see it in the spec..?

That is in the updated specs. I had spec updates in CSHARP-3660 mixed in with C# code fixes to make them work. But then the PR was going to be painful to review because there were thousands of spec changes (which had already been reviewed as part of DRIVERS-1293) mixed in with C# code changes. Some of those changes were necessary here to get tests passing. Rather than try to disentangle the C# changes necessary to fix the existing spec tests and the C# changes necessary to fix the updated spec tests, I just moved all the C# changes to this ticket for easier review.

my question was about this folder legacy_hello that I didn't find in the spec master. Also If this is a folder with old tests, I would expect naming like legacy similar to we have in other places (maybe it's out of the scope here)

DmitryLukyanov · 2021-06-12T16:15:56Z

...sts/Specifications/server-discovery-and-monitoring/ServerDiscoveryAndMonitoringTestRunner.cs

-                return base.ShouldReadJsonDocument(path) && !path.StartsWith(MonitoringPrefix);
+                return base.ShouldReadJsonDocument(path) &&
+                       !MonitoringPrefixes.Any(prefix => path.StartsWith(prefix)) &&
+                       !path.EndsWith("discover_load_balancer.json");  // load balancer support not yet implemented


it's better to disable the whole folder rather than individual files

Fair enough point. I'll skip the folder instead.

DmitryLukyanov · 2021-06-12T19:00:01Z

tests/MongoDB.Driver.Tests/JsonDrivenTests/JsonDrivenTestFactory.cs

@@ -114,6 +115,7 @@ public JsonDrivenTest CreateTest(string receiver, string name)
                    switch (name)
                    {
                        case "listDatabaseNames": return new JsonDrivenListDatabaseNamesTest(_client, _objectMap);
+                        case "listDatabaseObjects": throw new SkipException(".NET/C# driver does not implement a ListDatabaseObjects helper.");


where this option come from? I don't see it in the specs

This option is coming from the updated retryable reads spec tests, which will be merged after this PR.

DmitryLukyanov · 2021-06-12T19:00:38Z

tests/MongoDB.Driver.Tests/JsonDrivenTests/JsonDrivenTestFactory.cs

@@ -138,6 +140,7 @@ public JsonDrivenTest CreateTest(string receiver, string name)
                        case "createCollection": return new JsonDrivenCreateCollectionTest(database, _objectMap);
                        case "dropCollection": return new JsonDrivenDropCollectionTest(database, _objectMap);
                        case "listCollectionNames": return new JsonDrivenListCollectionNamesTest(database, _objectMap);
+                        case "listCollectionObjects": throw new SkipException(".NET/C# driver does not implement a ListCollectionObjects helper.");


the same as above, I don't see this key in the spec

This option is coming from the updated retryable reads spec tests, which will be merged after this PR.

well, this option looks like this one: https://github.com/mongodb/specifications/blob/master/source/unified-test-format/unified-test-format.rst#listcollections (that I already implemented in load balanced PR), is it different step?

DmitryLukyanov · 2021-06-12T19:01:04Z

tests/MongoDB.Driver.Tests/JsonDrivenTests/JsonDrivenTestFactory.cs

@@ -168,6 +171,7 @@ public JsonDrivenTest CreateTest(string receiver, string name)
                        case "insertMany": return new JsonDrivenInsertManyTest(collection, _objectMap);
                        case "insertOne": return new JsonDrivenInsertOneTest(collection, _objectMap);
                        case "listIndexes": return new JsonDrivenListIndexesTest(collection, _objectMap);
+                        case "listIndexNames": throw new SkipException(".NET/C# driver does not implement a ListIndexNames helper.");


the same as above

This option is coming from the updated retryable reads spec tests, which will be merged after this PR.

DmitryLukyanov · 2021-06-12T19:02:31Z

...cations/server-discovery-and-monitoring/ServerDiscoveryAndMonitoringIntegrationTestRunner.cs

@@ -18,7 +18,6 @@
 using System.Threading.Tasks;
 using MongoDB.Bson;
 using MongoDB.Bson.TestHelpers.JsonDrivenTests;
-using MongoDB.Driver;


nit: we usually don't leave such small changes

It was an unused using. Figured I'd remove it since I was making other edits in the same set of files.

we don't do it because it makes a git history a bit messy (you didn't make any valuable changes in the file but you see this file in the log), but it's minor anyway

DmitryLukyanov · 2021-06-12T19:03:21Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedTestRunner.cs

@@ -51,6 +51,11 @@ public sealed class UnifiedTestRunner : IDisposable

        public void Run(JsonDrivenTestCase testCase)
        {
+            if (testCase.Name.Contains("mongos-unpin.json"))


can we add this on the particular runner level?

Good suggestion. Done.

JamesKovacs · 2021-06-14T17:21:10Z

Should we make anything with isMasterResult also? like wrap it to HelloResult and use a new class in the code instead. Will we be able just to remove isMasterResult in 3.0 without depricating this class now?

Refactoring isMasterResult is being done as part of CSHARP-3660. It's coming, but not done quite yet.

DmitryLukyanov

LGTM

CSHARP-3705: Ignore hello commands during event capture for UnifiedTe…

f43cda7

…stRunner.

JamesKovacs requested a review from jyemin June 10, 2021 21:07

jyemin approved these changes Jun 10, 2021

View reviewed changes

CSHARP-3705: Updates to spec testing code to handle hello and ignore …

7695885

…tests related to not-yet-implemented load balancer support.

jyemin requested a review from DmitryLukyanov June 11, 2021 21:24

CSHARP-3705: Updated sharded cluster test to reflect new explain plan…

444accf

… output from 5.0 sharded clusters.

DmitryLukyanov reviewed Jun 12, 2021

View reviewed changes

Incorporated PR feedback.

ef8b66d

JamesKovacs requested a review from DmitryLukyanov June 14, 2021 18:09

DmitryLukyanov approved these changes Jun 14, 2021

View reviewed changes

JamesKovacs merged commit b370f5f into mongodb:master Jun 14, 2021

JamesKovacs deleted the csharp3705 branch June 14, 2021 20:31

CSHARP-3705: Ignore hello commands during event capture for UnifiedTestRunner. #548

CSHARP-3705: Ignore hello commands during event capture for UnifiedTestRunner. #548

Uh oh!

Conversation

JamesKovacs commented Jun 10, 2021

Uh oh!

jyemin left a comment

Choose a reason for hiding this comment

Uh oh!

JamesKovacs commented Jun 11, 2021

Uh oh!

JamesKovacs commented Jun 11, 2021

Uh oh!

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

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!

JamesKovacs commented Jun 14, 2021

Uh oh!

DmitryLukyanov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!