Downgrade Xunit, Improve exception handling for Diagnonstic Tests

[benrr101]

Description: While doing some investigation into improving the diagnostic tracing tests, I noticed that test failures are logged very non-descriptly by remote executor. To resolve this issue, we have to downgrade xunit to v2.4.2. Here's the rationale for this change:

• RemoteExecutor throws XunitExceptions from xunit.assert nuget and uses the (string, string) constructor
• The (string, string) constructor was removed in >=2.5 of xunit.assert
    * RemoteExecutor was fixed to inherit from Exception in newer versions, but for us to take this requires upgrading to .net 7 or 8 (which I imagine is a much larger lift)
• In order to downgrade xunit.assert, we need to downgrade xunit
• In order to downgrade xunit, we also need to downgrade Microsoft.DotNet.XunitExtensions
• Without this change, exceptions from RemoteExecutor will fail to construct, masking all errors with "MethodNotFound"

Although normally downgrading is bad for security reasons, I believe this to be a safe change since it only affects tests.

This PR also makes a bunch of improvements to the DiagnosticTests:

• Remove conditional facts when test TDS server is used
• Remove try/catch with empty catch block, replace with assert.throws
• Ensure that connections are opened before trying test conditions
• Document scenarios where test TDS server doesn't obey actual behavior, or where test TDS server cannot be used
• Formatting improvements

Testing:

• All tests (that are expected to pass) continue to pass
• In a synthetic failure of diagnostic tests, here's the before and after of the error messages:
  • Before:

    System.MissingMethodException: Method not found: 'Void Xunit.Sdk.XunitException..ctor(System.String, System.String)'.
    
    System.MissingMethodException
    Method not found: 'Void Xunit.Sdk.XunitException..ctor(System.String, System.String)'.
    

  • After:

    Microsoft.DotNet.RemoteExecutor.RemoteExecutionException: Remote process failed with an unhandled exception.
    
    Microsoft.DotNet.RemoteExecutor.RemoteExecutionException
    Remote process failed with an unhandled exception.
    Child exception:
      Xunit.Sdk.FailException: Assert.Fail(): Foo
       at Microsoft.Data.SqlClient.ManualTesting.Tests.DiagnosticTest.<>c.<SyntheticFailure>b__1_0() in 
    C:\Projects\benrr101\sqlclient\src\Microsoft.Data.SqlClient\tests\ManualTests\TracingTests\DiagnosticTest.cs:line 34
    

[JRahnama]

I am not a fan of this change since it is downgrading the version. If there is a missing functionality we need to reach out to xunit maintainers and ask them for an update or to add a functionality. Plus, since net6 will be out of support at the end of this year, I am not sure how the compatibility will work on lower versions of xunit and net8 and net9. We may end up upgrading that again which will make it redundant.

[David-Engel]

I'm on the fence about the version downgrades. At a high level, for the next MDS release we want to:

1. Bump to v6.0.
2. Remove .NET Standard support.
3. Remove .NET 6 support.
4. Switch the netfx project to .NET SDK style.

So upgrading to .NET 8 can come sooner than later.

I don't see MDS 6.0 GAing before .NET 6 EOLs Nov 12, so I think it would be safe to drop it for all the previews. If you have bandwidth to take on any of these larger tasks, that would be great. We will need to coordinate pipeline changes with 2 and 3, which may be made easier after the migration to YAML pipelines (in progress). Once they are in YAML, pipeline and code changes can be part of the same PR.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.63%. Comparing base (5376a04) to head (adad96a).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2379      +/-   ##
==========================================
+ Coverage   72.60%   72.63%   +0.02%     
==========================================
  Files         310      310              
  Lines       61885    61885              
==========================================
+ Hits        44931    44949      +18     
+ Misses      16954    16936      -18     

Flag	Coverage Δ
addons	92.88% <ø> (ø)
netcore	76.84% <ø> (+0.05%)	⬆️
netfx	70.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ErikEJ]

@David-Engel I have a draft branch with .netstandard Removal. Maybe I will leave until yaml pipeline is ready?

[benrr101]

@JRahnama In this case, there isn't anything required of xunit - dotnet/arcade has already fixed the issue by having the remote executor exception inherit from Exception directly. However, we cannot take that newer version because the package it requires .net 7 or 8. As much as I'd like to just bump the test project versions up, I presume that will break many things.

@David-Engel So, it sounds like there's no interest in taking downgrading xunit to improve error messages on remote executor tests? Basically, hold on until we can upgrade everything to net7.0+? If that's the case, I'll at least resubmit the improvements to the diagnostic tests.

[David-Engel]

So, it sounds like there's no interest in taking downgrading xunit to improve error messages on remote executor tests? Basically, hold on until we can upgrade everything to net7.0+? If that's the case, I'll at least resubmit the improvements to the diagnostic tests.

Like I said, I'm on the fence. I'm fine with it as long as we don't forget to bump it back up once we remove .NET 6.

[ErikEJ]

Maybe an issue to upgrade should be created?

[JRahnama]

In this case, there isn't anything required of xunit - dotnet/arcade has already fixed the issue by having the remote executor exception inherit from Exception directly. However, we cannot take that newer version because the package it requires .net 7 or 8. As much as I'd like to just bump the test project versions up, I presume that will break many things.

I was wondering, if we could have different versions of xunit based on our versionprops for different TFM?

[benrr101]

@David-Engel Understood, thanks for the approval. If it were me, when the upgrade to net7.0+ comes, I'd also do a sweep to upgrade all packages in the non-product projects (eg, tests). But I suppose I can create an issue to upgrade as per @ErikEJ 's suggestion?

@JRahnama Perhaps, but I need a bit of help to understand. What's TFM? I'm also having difficulty in tracing all the conditionals that go into building the tests. My gut feeling is we should reduce the number of conditionals in the projects/props to make it simpler to manage. If the conditions we would split xunit versions on already exists, let me know the required changes and I'll update this PR. But, if we'd need to introduce more conditionals to make it happen, I think we should probably avoid that.

[JRahnama]

Perhaps, but I need a bit of help to understand. What's TFM?

Certainly.

Recently, we introduced VersionsNet8OrLater.props for .NET 8 and later versions. Essentially, if we have a different dependency version for .NET 8, the values are read from this file. You can add a newer version of xunit and xunitextension from the Arcade to that folder and update the one in Versions.props to the lower version. In this scenario, .NET 8 could utilize the newer version of xunit, while .NET 6 will utilize the lower version. You don't need to perform any additional steps; just add different versions in different folders, and theoretically that should suffice.

TFM -> Target Framework Monikers

[benrr101]

@JRahnama Added the newer version of xunit (the original one) and the latest version of the arcade nugets to the .net8+ version props file. I think 😅 I'm a little unsure how to build specifically for .net8 versus .net6, tho. Let me know if I did something wrong or how I can validate it better on my end, please. Thanks!

[JRahnama]

I'm a little unsure how to build specifically for .net8 versus

msbuild -t:BuildTestsNetCore -p:TF=net8.0 -p:Configuration= <Release or Debug>

[JRahnama]

I think line 9 is redundant.

[benrr101]

Maybe not redundant, but I suppose outside the scope, since we don't need to upgrade xunit extensions today.

[benrr101]

@JRahnama How's it look now?

[JRahnama]

@benrr101 LGTM. I will do some testing today and will approve it after. Thank you.