CSHARP-4400: Upgrade xunit to 2.4.2 #935

BorisDog · 2022-11-07T15:37:25Z

SkippableTests removed

BorisDog · 2022-11-07T15:37:57Z

CSharpDriver.sln

@@ -1,7 +1,7 @@

 Microsoft Visual Studio Solution File, Format Version 12.00
-# Visual Studio Version 16
-VisualStudioVersion = 16.0.30011.22
+# Visual Studio Version 17


Good opportunity to update VS version as well?

=). I did the same here: https://github.com/mongodb/mongo-csharp-driver/pull/934/files#diff-915489976f8122b3863d80c3d6d98569674566a1d9d7a9080796e16cbac7ecd2R4

DmitryLukyanov · 2022-11-07T15:42:30Z

CSharpDriver.sln

 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "MongoDB.Driver.TestConsoleApplication", "tests\MongoDB.Driver.TestConsoleApplication\MongoDB.Driver.TestConsoleApplication.csproj", "{2E5780D2-29A5-483C-9CA2-844F45A66D0C}"
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "AstrolabeWorkloadExecutor", "tests\AstrolabeWorkloadExecutor\AstrolabeWorkloadExecutor.csproj", "{B90F025F-89D3-436A-AD78-6AA304A6E240}"
 EndProject
-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "MongoDB.Driver.SmokeTests.Sdk", "tests\SmokeTests\MongoDB.Driver.SmokeTests.Sdk\MongoDB.Driver.SmokeTests.Sdk.csproj", "{B711A69F-A337-452C-95E1-A6B15C727CBA}"
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "MongoDB.Driver.SmokeTests.Sdk", "tests\SmokeTests\MongoDB.Driver.SmokeTests.Sdk\MongoDB.Driver.SmokeTests.Sdk.csproj", "{B711A69F-A337-452C-95E1-A6B15C727CBA}"


I saw this diff too, but didn't really understand why it exists and just ignored it

DmitryLukyanov

Question and then LGTM

DmitryLukyanov · 2022-11-08T18:26:26Z

...r.Core.TestHelpers/XunitExtensions/TimeoutEnforcing/TimeoutEnforcingXunitTestMethodRunner.cs

@@ -26,7 +26,7 @@ namespace MongoDB.Driver.Core.TestHelpers.XunitExtensions.TimeoutEnforcing
    [DebuggerStepThrough]
    internal sealed class TimeoutEnforcingXunitTestMethodRunner : XunitTestMethodRunner
    {
-        private static string[] __skippingExceptionNames = new string[] { typeof(SkipException).FullName };
+        private static string[] __skippingExceptionNames = new string[] { typeof(Xunit.SkipException).FullName };


why do we need this change?

To distinguish from Xunit.Sdk.SkipException that was introduced in 2.4.2. (we use Xunit.SkipException from Xunit.SkippableFact package).

If xUnit now has SkipException why do we still need the SkippableFact nuget package?

Good question.
That was my line of thought as well, and I even removed SkippableFact nuget package. Only to discover that raising Xunit.Sdk.SkipException marks test as failed and not skipped.

Maybe xUnit now has a different way than [SkippableFact] to mark a test as skippable?

I did not find any. The only way I found is to specify Skip reason in the Fact attribute.
Also SkipException is not referenced anywhere directly in xunit main repository (defined xunit/asserts), so looks like there is not special handling for it.

hmm, I even can't create Sdk.SkipException (ie I don't see it from the code). Do you observe different behavior?

Yes, new Xunit.Sdk.SkipException(...) works.

yeah, I didn't see this class until I remove all bin/obj folders

DmitryLukyanov · 2022-11-09T14:38:48Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

@@ -47,6 +46,7 @@
 using Xunit.Abstractions;
 using Xunit.Sdk;
 using Reflector = MongoDB.Bson.TestHelpers.Reflector;
+using SkipException = Xunit.SkipException;


if SDK.SkipException is possible to create (see my another comment) and we support only the previous exception version, I would prefer to stick to the version where we don't have 2 different exceptions for the same goal. Ie we won't need to worry about such change each time when we want to throw SkipException. Can you remind me why we want this xunit upgrading?

We just need to Xunit.SkipException exception which is defined in Xunit.SkippableFact .
The goal is to better support generics, and looks like there are few bugs fixes in past two years.

I still think that the existence of TWO versions of SkipException is a problem. At the very least it's confusing. More likely it's a sign that there is duplicate functionality and we should be able to drop our dependency on the SkippableFact nuget package.

As mentioned above, I've investigated the possibility for dropping SkippableFact package. I have not been able to find the matching functionality in any V2 version of xUnit. I'll happy to be mistaken here.
I don't think it's a real problem, as we very rarely use SkipException directly.

We probably need to spend more time on this. I think it's clear that a new SkipException should be used somehow, it's definitely not added just to confuse people. Maybe it's something about this directive https://github.com/xunit/assert.xunit/blob/b78867e1aa4b2aa255b5dfc577f6014812445adb/Sdk/Exceptions/SkipException.cs#L10 that can be configured via csproj..

so it's not relevant for us.

you're right, we're working with complied binaries.

TimeoutEnforcingXunitTestMethodRunner. So that area will require some attention.

that was my main concern initially, so not sure how easy it will be

We can do this investigation in a follow up ticket.

How much urgent this change now? I would say not really and there is no rush with pushing these changes now. @rstam what do you think?

I think we are two unrelated issues here:

Enabling generics in tests with this PR

Removing SkippableFact package (which from my research can't be done yet)

I think we can safely and easily achieve the original goal of this PR and deal with the second improvement when needed.

From what I see, Enabling generics in tests with this PR is not too urgent issue to introduce code weirdness. We can simply wait until we will figure out the right and not confusing way to do it.

My estimation is that we will be able to remove the SkippableFact only when upgrading to v3 xUnit or other testing framework.
Having generics support now would be quite convenient, while code weirdness will be practically unnoticed.

Added Xunit.Sdk.SkipException as skippable exception as well. Now either exception will skip the test.

JamesKovacs

While these changes make sense and clean up the project quite nicely, something isn't quite right. If you look at the patch build, all base statuses of Skip are now Failed. Something isn't working correctly on Evergreen even if it is working locally.

JamesKovacs · 2022-11-14T22:18:51Z

tests/MongoDB.Bson.TestHelpers/MongoDB.Bson.TestHelpers.csproj

    <PackageReference Include="JunitXml.TestLogger" Version="2.1.81" />
+    <PackageReference Include="Xunit.SkippableFact" Version="1.4.13">
+      <PrivateAssets>all</PrivateAssets>
+    </PackageReference>


I don't understand this change to the package reference. What does PrivateAssets do?

Hides this reference from dependent projects.
For example: otherwise SkippableFact will be available in MongoDB.Driver.Tests.

Ah ok. That makes sense.

So the only remaining question is why all the skipped tests are failing on Evergreen.

DmitryLukyanov · 2022-11-30T12:22:48Z

tests/MongoDB.Bson.TestHelpers/XunitExtensions/TimeoutEnforcing/ITestExceptionHandler.cs

@@ -0,0 +1,9 @@
+using System;


no copyright

DmitryLukyanov · 2022-11-30T13:11:01Z

tests/MongoDB.Bson.TestHelpers/XunitExtensions/TimeoutEnforcing/SkippableTestMessageBus.cs

+        /// <inheritdoc />
+        public bool QueueMessage(IMessageSinkMessage message)
+        {
+            var failed = message as TestFailed;


is it a copy of xunit logic? if no, what the point in this class?

No, this is functionality implemented in Xunit.SkippableFact (not xUnit)
It allows to skip the test when SkipException is raised.

DmitryLukyanov · 2022-11-30T13:14:17Z

tests/MongoDB.Bson.TestHelpers/XunitExtensions/TimeoutEnforcing/TimeoutEnforcingTestInvoker.cs

 {
    [DebuggerStepThrough]
    internal sealed class TimeoutEnforcingTestInvoker : XunitTestInvoker
    {
+        private struct YieldNoContextAwaitable


I'm not a big fan of copying this class from the driver, but probably effective change here is placing this helper on some common level which only temporary is Bson level, so is it correct that this logic will be moved from here with some later refactoring?

Yes exactly, moving the TaskHelpers was comlicated. Do you think there should be a comment for that in the code?

yeah, comment would be good

DmitryLukyanov · 2022-11-30T13:15:12Z

tests/MongoDB.Driver.Core.TestHelpers/Logging/LoggableTestClass.cs

@@ -28,7 +29,7 @@
 namespace MongoDB.Driver.Core.TestHelpers.Logging
 {
    [DebuggerStepThrough]
-    public abstract class LoggableTestClass : IDisposable, ILoggingService
+    public abstract class LoggableTestClass : IDisposable, ILoggingService, ITestExceptionHandler


I like this change

DmitryLukyanov · 2022-11-30T13:15:48Z

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

@@ -1,8 +0,0 @@
-namespace MongoDB.Driver.Core.TestHelpers.XunitExtensions


copyright :)

Removed this file.

DmitryLukyanov · 2022-11-30T13:17:26Z

tests/MongoDB.Driver.Tests/Specifications/Runner/MongoClientJsonDrivenTestRunnerBase.cs

@@ -613,7 +614,7 @@ internal static class MongoClientSettingsReflection
    {
 #pragma warning disable CS0618 // Type or member is obsolete
        public static void _connectionModeSwitch(this MongoClientSettings obj, ConnectionModeSwitch connectionModeSwitch)
-            => Reflector.SetFieldValue(obj, nameof(_connectionModeSwitch), connectionModeSwitch);
+            => Bson.TestHelpers.Reflector.SetFieldValue(obj, nameof(_connectionModeSwitch), connectionModeSwitch);


move prefix to using (same in other places if any)?

It conflicts with Xunit.Sdk.Refelector

got it, can we use xunit.sdk reflector instead of our implementation? Probably unrelated to current changes question :)

No it is quite different. Also I would not recommend increasing our dependency in such areas on test framework.

DmitryLukyanov

LGTM. Assuming the rest comments are resolved.

JamesKovacs

A few questions / minor changes...

JamesKovacs · 2022-11-30T21:27:37Z

tests/MongoDB.Bson.TestHelpers/XunitExtensions/TimeoutEnforcing/TimeoutEnforcingTestInvoker.cs

@@ -56,10 +84,10 @@ protected override async Task<decimal> InvokeTestMethodAsync(object testClassIns
            var timeoutMS = xUnitTestCase?.Timeout ?? 0;
            var timeout = Debugger.IsAttached
                ? Timeout.InfiniteTimeSpan // allow more flexible debugging expirience
-                : timeoutMS <= 0 ? CoreTestConfiguration.DefaultTestTimeout : TimeSpan.FromMilliseconds(timeoutMS);
+                : timeoutMS <= 0 ? XunitExtensionsConsts.DefaultTestTimeout : TimeSpan.FromMilliseconds(timeoutMS);


This should be named XunitExtensionsConstants.

JamesKovacs · 2022-11-30T21:34:11Z

tests/MongoDB.Bson.TestHelpers/XunitExtensions/TimeoutEnforcing/ITestExceptionHandler.cs

+
+using System;
+
+namespace MongoDB.Bson.TestHelpers.XunitExtensions.TimeoutEnforcing


I'm not liking that these test helpers - which have nothing to do with BSON - are in the MongoDB.Bson namespace. I realize that BSON is the lowest common denominator, but I wonder if we should create a MongoDB.TestHelpers project to contain code such as this. Thoughts?

I like this idea!

JamesKovacs · 2022-11-30T21:35:23Z

tests/MongoDB.Bson.TestHelpers/XunitExtensions/XunitExtensionsConsts.cs

+
+namespace MongoDB.Bson.TestHelpers.XunitExtensions
+{
+    public static class XunitExtensionsConsts


XunitExtensionsConstants to follow our naming conventions.

JamesKovacs

LGTM

rstam

Overall looks great!

I have one small suggestion. See what you think. If you don't agree with it I'm happy to LGTM as is. Let me know and re-request my review.

rstam · 2022-12-09T22:01:32Z

tests/MongoDB.Bson.Tests/Serialization/Serializers/ObjectSerializerTests.cs

-        public static IDiscriminatorConvention _discriminatorConvention(this ObjectSerializer obj) => (IDiscriminatorConvention)Reflector.GetFieldValue(obj, nameof(_discriminatorConvention));
-        public static GuidRepresentation _guidRepresentation(this ObjectSerializer obj) => (GuidRepresentation)Reflector.GetFieldValue(obj, nameof(_guidRepresentation));
+        public static IDiscriminatorConvention _discriminatorConvention(this ObjectSerializer obj) => (IDiscriminatorConvention)TestHelpers.Reflector.GetFieldValue(obj, nameof(_discriminatorConvention));
+        public static GuidRepresentation _guidRepresentation(this ObjectSerializer obj) => (GuidRepresentation)TestHelpers.Reflector.GetFieldValue(obj, nameof(_guidRepresentation));


Damn you Xunit.Sdk.Reflector... :(

Consider putting this at the top of the file to disambiguate once:

using Reflector = MongoDB.Bson.TestHelpers.Reflector;

and the same in all other files where this happens.

Looks like the only other file where this happens is MongoClientJsonDrivenTestRunnerBase.cs

nit: since we already target c# 10, we can try setting global using: https://www.c-sharpcorner.com/article/global-using-directive-in-c-sharp-102/ (not sure whether it works in the way we need here though)

Not sure whether global using applies to the using Name = FullTypeName format.

Also not sure in general that we actually want global usings... I'd be pretty cautious about forcing a using statement into every single file. It would have to be a pretty limited set I would think.

I don't think the number of using statements in each file is a problem. It's not like we type them manually, Visual Studio adds them for us automatically (via Ctrl-.).

Good suggestion! Done.
I agree that making Reflector a global using is not a trivial decision.

rstam

LGTM I think?

You squashed and rebased and force pushed and I can no longer tell what changed since I last reviewed...

BorisDog · 2022-12-10T00:43:41Z

LGTM I think?

You squashed and rebased and force pushed and I can no longer tell what changed since I last reviewed...

Yes sorry, I spent some time on rebasing beforehand and it was just easier that way. The only new changes are namespaces resolutions with master and using Reflector = MongoDB.Bson.TestHelpers.Reflector;

…ongodb#935)

BorisDog requested a review from JamesKovacs November 7, 2022 15:37

BorisDog commented Nov 7, 2022

View reviewed changes

DmitryLukyanov reviewed Nov 7, 2022

View reviewed changes

JamesKovacs requested review from DmitryLukyanov and removed request for JamesKovacs November 7, 2022 22:49

DmitryLukyanov approved these changes Nov 8, 2022

View reviewed changes

DmitryLukyanov reviewed Nov 9, 2022

View reviewed changes

BorisDog requested a review from DmitryLukyanov November 10, 2022 23:30

JamesKovacs requested changes Nov 14, 2022

View reviewed changes

BorisDog requested a review from JamesKovacs November 24, 2022 16:36

DmitryLukyanov reviewed Nov 30, 2022

View reviewed changes

DmitryLukyanov self-requested a review November 30, 2022 13:39

DmitryLukyanov approved these changes Nov 30, 2022

View reviewed changes

JamesKovacs requested changes Nov 30, 2022

View reviewed changes

BorisDog force-pushed the csharp4400 branch from 7cb6391 to ea1bb03 Compare December 1, 2022 18:17

BorisDog requested a review from JamesKovacs December 1, 2022 19:01

JamesKovacs approved these changes Dec 9, 2022

View reviewed changes

rstam requested changes Dec 9, 2022

View reviewed changes

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

fadd421

BorisDog force-pushed the csharp4400 branch from ea1bb03 to fadd421 Compare December 10, 2022 00:15

BorisDog requested a review from rstam December 10, 2022 00:18

rstam approved these changes Dec 10, 2022

View reviewed changes

BorisDog merged commit 2d60145 into mongodb:master Dec 12, 2022

BorisDog added a commit to BorisDog/mongo-csharp-driver that referenced this pull request Dec 22, 2022

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

40b011d

…ongodb#935)

BorisDog added a commit to BorisDog/mongo-csharp-driver that referenced this pull request Jan 30, 2023

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

329ec12

…ongodb#935)

dnickless pushed a commit to dnickless/mongo-csharp-driver that referenced this pull request Aug 24, 2023

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

616a80e

…ongodb#935)

		@@ -1,8 +0,0 @@
		namespace MongoDB.Driver.Core.TestHelpers.XunitExtensions


		using System;

		namespace MongoDB.Bson.TestHelpers.XunitExtensions.TimeoutEnforcing

CSHARP-4400: Upgrade xunit to 2.4.2 #935

CSHARP-4400: Upgrade xunit to 2.4.2 #935

Uh oh!

Conversation

BorisDog commented Nov 7, 2022

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!

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!

BorisDog Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

DmitryLukyanov Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

DmitryLukyanov Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

DmitryLukyanov Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BorisDog Nov 8, 2022 •

edited

Loading

DmitryLukyanov Nov 9, 2022 •

edited

Loading

DmitryLukyanov Nov 9, 2022 •

edited

Loading

DmitryLukyanov Nov 30, 2022 •

edited

Loading

BorisDog Nov 30, 2022 •

edited

Loading

DmitryLukyanov left a comment •

edited

Loading