Fix | Fix unit test for SPN to include port number with Managed SNI #2281

arellegue · 2023-12-21T22:28:55Z

This PR is for the Unit Test for the fix of Issue #2187

This PR contains the unit test for the GetSqlServerSPN method inside MDS driver where when a tcp protocol and named instance is used in the data source, it used to return the named instance name instead of the port number.

This PR's unit test is testing TCP protocol with a Named Instance with a port number in a data source specified.

…d Named Instance with or without port number.

…t to fix intermittent issue in the pipeline.

…NI dll.

…oo long.

…its the first annotation as the unit test did not skip.

…unning in pipeline. Added conditional compilation instead.

codecov · 2023-12-22T03:50:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.67%. Comparing base (5e07b29) to head (0c5ed6f).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2281      +/-   ##
==========================================
+ Coverage   72.61%   72.67%   +0.05%     
==========================================
  Files         310      310              
  Lines       61877    61875       -2     
==========================================
+ Hits        44935    44968      +33     
+ Misses      16942    16907      -35

Flag	Coverage Δ
addons	`92.88% <ø> (ø)`
netcore	`76.94% <100.00%> (+0.08%)`	⬆️
netfx	`70.14% <ø> (-0.04%)`	⬇️

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.

…sitive.

H-Yeo

Minor things to me. More around consistency and readability.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs

H-Yeo

Minor things to me. More around consistency and readability.

H-Yeo

Aside from my last re-mark. Looks good to me.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

…sed Assert.Fail.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

Rename variables to what they are. Changed ConditionalTheory to ConditionalFact and get the port number of named instance from Sql Browser.

…ort number agains port number returned in SPN.

H-Yeo

I'm finished with my review. Just some comments on things I missed the first time around. Please review, make changes as necessary.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

H-Yeo · 2024-02-01T16:46:03Z

LGTM

arellegue · 2024-02-01T16:59:59Z

LGTM

Are you going to approve?

H-Yeo

Left an optional fix. I know this has been sitting here for awhile, and this is for a test. The only concern I have is reflection for the test, which I'm not an expert on. The 2nd reviewer/approver may have more changes to the usage of reflection.

H-Yeo · 2024-02-05T21:41:23Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

+            int port = 0;
+
+            bool isDataSourceValid = DataTestUtility.ParseDataSource(builder.DataSource, out hostname, out _, out instanceName);
+            Assert.True(isDataSourceValid, "DataSource is invalid");


[Optional] - Not a deal breaker for me. But this is missing a period for consistency.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

…2436)

arellegue added 11 commits December 18, 2023 12:10

Remove ActiveIssue to test pipeline.

9c776f3

Fix SPN port number Unit Test to use TCP and NP connection strings an…

f84884c

…d Named Instance with or without port number.

Try adding retry and delay in creating and using the connection objec…

36c235f

…t to fix intermittent issue in the pipeline.

Added IsUsingManagedSNI annotation since reflection is using ManagedS…

0caa4ce

…NI dll.

Add wrapper for all required annotations for the unit test as it is t…

c47a16e

…oo long.

Removed all Sleeps as they are not needed.

64c0103

Add annotation to skip .net framework for SPN port unit test.

af685d3

Put [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] so …

6076e8e

…its the first annotation as the unit test did not skip.

Removed SkipOnTargetFramework as it did not stop the unit test from r…

bea5513

…unning in pipeline. Added conditional compilation instead.

Add validation of InstanceName within the ConditionalThreory annotation.

fa556f5

Removed and sorted using references.

6ace809

arellegue requested review from David-Engel, JRahnama and DavoudEshtehari December 21, 2023 22:28

Fix ParseDataSource named pipe data source detection to be case insen…

660d992

…sitive.

H-Yeo self-requested a review January 5, 2024 00:15

H-Yeo reviewed Jan 5, 2024

View reviewed changes

Applied PR review suggesstions.

192a1e2

H-Yeo reviewed Jan 10, 2024

View reviewed changes

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs Outdated Show resolved Hide resolved

Moved named pipe protocol port usage assert message to line 138 and u…

8a93097

…sed Assert.Fail.

DavoudEshtehari added the Ⓜ️ Managed SNI Use this label if the issue/PR relates to issues in Managed SNI label Jan 15, 2024

DavoudEshtehari reviewed Jan 16, 2024

View reviewed changes

Removed all source codes that handle named pipe protocol.

00d9a3c

DavoudEshtehari reviewed Jan 19, 2024

View reviewed changes

kf-gonzalez added this to the 5.2.0 milestone Jan 24, 2024

H-Yeo modified the milestones: 5.2.0, 5.2.0-preview5 Jan 24, 2024

arellegue added 2 commits January 24, 2024 12:49

Removed unwanted comments as they cause code bloat.

41366f6

Rename variables to what they are. Changed ConditionalTheory to ConditionalFact and get the port number of named instance from Sql Browser.

Add SPN pattern validation and use Assert.Equal to compare expected p…

baa3ab9

…ort number agains port number returned in SPN.

arellegue requested review from DavoudEshtehari and H-Yeo January 25, 2024 18:43

H-Yeo reviewed Feb 1, 2024

View reviewed changes

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs Outdated Show resolved Hide resolved

arellegue added 3 commits January 31, 2024 17:01

Merge branch 'main' into FixUnitTestForSPNPortNumber

53a13e6

Added validation for DataSource.

2bc3f7c

Added assertion if DataSource is valid.

c131562

arellegue requested a review from H-Yeo February 1, 2024 16:30

H-Yeo approved these changes Feb 5, 2024

View reviewed changes

JRahnama reviewed Feb 23, 2024

View reviewed changes

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs Outdated Show resolved Hide resolved

DavoudEshtehari requested a review from H-Yeo February 23, 2024 18:05

DavoudEshtehari approved these changes Feb 23, 2024

View reviewed changes

Replace If (port > 0) with Assert( port > 0, ...)

0c5ed6f

arellegue requested a review from JRahnama February 23, 2024 18:32

JRahnama approved these changes Feb 26, 2024

View reviewed changes

DavoudEshtehari merged commit 94c089e into dotnet:main Feb 26, 2024
148 checks passed

JRahnama added a commit to JRahnama/SqlClient that referenced this pull request Mar 7, 2024

Fix | Revert back PR dotnet#2281 code changes

64d48a3

JRahnama added a commit to JRahnama/SqlClient that referenced this pull request Mar 8, 2024

Regression | Revert PR dotnet#2281 SNIProxy code change

e714394

JRahnama mentioned this pull request Mar 8, 2024

Regression | Revert PR #2281 SNIProxy code change #2395

Merged

DavoudEshtehari pushed a commit that referenced this pull request Mar 25, 2024

Regression | Revert PR #2281 SNIProxy code change (#2395)

bdfa5d8

DavoudEshtehari added a commit that referenced this pull request Mar 28, 2024

Regression | Revert PR #2281 SNIProxy code change (#2395)

f50c866

DavoudEshtehari added a commit that referenced this pull request Mar 28, 2024

Regression | Revert PR #2281 SNIProxy code change (#2395)

21e10ae

DavoudEshtehari added a commit that referenced this pull request May 8, 2024

[5.2.1] | Regression | Revert PR #2281 SNIProxy code change (#2395) (#…

51d58fa

…2436)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix | Fix unit test for SPN to include port number with Managed SNI #2281

Fix | Fix unit test for SPN to include port number with Managed SNI #2281

arellegue commented Dec 21, 2023 •

edited

codecov bot commented Dec 22, 2023 •

edited

H-Yeo left a comment

H-Yeo left a comment

H-Yeo left a comment

H-Yeo left a comment

H-Yeo commented Feb 1, 2024

arellegue commented Feb 1, 2024

H-Yeo left a comment

H-Yeo Feb 5, 2024

Fix | Fix unit test for SPN to include port number with Managed SNI #2281

Fix | Fix unit test for SPN to include port number with Managed SNI #2281

Conversation

arellegue commented Dec 21, 2023 • edited

codecov bot commented Dec 22, 2023 • edited

Codecov Report

H-Yeo left a comment

Choose a reason for hiding this comment

H-Yeo left a comment

Choose a reason for hiding this comment

H-Yeo left a comment

Choose a reason for hiding this comment

H-Yeo left a comment

Choose a reason for hiding this comment

H-Yeo commented Feb 1, 2024

arellegue commented Feb 1, 2024

H-Yeo left a comment

Choose a reason for hiding this comment

H-Yeo Feb 5, 2024

Choose a reason for hiding this comment

arellegue commented Dec 21, 2023 •

edited

codecov bot commented Dec 22, 2023 •

edited