Skip to content

Conversation

JRahnama
Copy link
Contributor

@JRahnama JRahnama commented Jan 15, 2021

While trying to test the driver against Net5, runtime caught some unobserved issues/warnings

  1. Classes implementing IDisposable should dispose all related fields.
  2. Re-Throwing caught exception from catch block will change the stack, hence should not be done.
  3. Calling overridable methods from constructions are prohibited.
  4. Raising exception from unexpected places such as dispose is not recommended.
  5. BinaryFormatter is not safe and should not be used in all versions of .NETFramework and netcoreapp 2.1 and upper versions.
  6. Warning CA1416: Platform compatibility this issue came up for tests running on Windows for CspParameters class. In net5.0 and later versions OperatindSystem.IsWindows
    is introduced to get the operating system but the API is not known in lower versions.

In some cases such as overridable methods usage I have moved the method inside a concrete method and called it from there and that made the driver to compile and run. I am eager to hear all comments about the changes to improve them and make them better.

Thanks

Update:

I noticed some improvement opportunities in our test lab:

SqlNotificationTest was using ManualResetEvent I have changed them to use ManualResetEventSlim. Reasoning to do so:

  1. ManualResetEventSlim is less expensive than ManualResetEvent.
  2. ManualResetEventSlim is fully managed version of its predecessor.

there are small differences such as kernel related behavior and blocking the thread which may have better effect on our test lab.

Copy link
Contributor

@Wraith2 Wraith2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny suggestions otherwise LGTM.

@cheenamalhotra cheenamalhotra added this to the 3.0.0-preview1 milestone Jan 15, 2021
@cheenamalhotra cheenamalhotra added the Area\Tests Issues that are targeted to tests or test projects label Jan 20, 2021
@cheenamalhotra cheenamalhotra self-requested a review January 29, 2021 21:14
@cheenamalhotra cheenamalhotra self-requested a review February 23, 2021 18:25
@cheenamalhotra
Copy link
Member

/azp run CI-Ub16-SQL17L-TR, CI-Win10-AzureSQLDB, CI-Win10-SQL17W, CI-Win10-SQL19W-Fx48-Core31, CI-Win10-SQL19W-ManagedSNI, CI-Win10-SQL19W-TR

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@cheenamalhotra cheenamalhotra merged commit ce4605d into dotnet:master Mar 2, 2021
@JRahnama JRahnama deleted the net5-tests-compatibility branch July 6, 2021 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area\Tests Issues that are targeted to tests or test projects

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants