Skip to content

Commit

Permalink
Downgrade Xunit, Improve exception handling for Diagnonstic Tests (#2379
Browse files Browse the repository at this point in the history
)

**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 `XunitException`s 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 `DiagnosticTest`s:
  * 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
    ```

## Commits
* Downgrade xunit for better error logging from remote executor

* All but one test improved ... ?

* Understanding why test was failing

* Add arcade and xunit nugets to .net8+ special versions

* Remove xunitextensions version boost (which I spelled wrong anyhow)
  • Loading branch information
benrr101 committed Mar 12, 2024
1 parent 769b982 commit 9056f13
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 134 deletions.
Loading

0 comments on commit 9056f13

Please sign in to comment.