Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tests | Activate "ActiveIssue" tests #3012

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Tests | Activate "ActiveIssue" tests #3012

wants to merge 33 commits into from

Conversation

MichelZ
Copy link
Contributor

@MichelZ MichelZ commented Nov 18, 2024 •
edited
Loading

This PR activates a couple of tests that were marked with "ActiveIssue"
Upon trying them out, they seem to run fine.

Also does some minor improvements on a few tests or disable some tests on specific platform

Fixes #3035

This also fixes and allows to close the following ActiveIssue's in AzDO:

  • 5531
  • 5533
  • 5535
  • 5536
  • 5541
  • 5540
  • 6643
  • 9196
  • 12161
  • 12161
  • 14588
  • 14325
  • 14590
  • 20245
  • 21707
  • 25147
  • 25421
  • 31754 (Partly - 2 tests activated, at least one remains)

MichelZ added 13 commits November 18, 2024 13:26
@MichelZ
Use Assert.Throws instead of home-grown
aef7d06
@MichelZ
TestConnectionStateWithErrorClass20 on NET8 fails on Debug build with…
d14975e 
… "incorrect initial ParseSynonyms size"
@MichelZ
Activate SqlExceptionTest, they seem to work fine now on Linux
0cd7a63
@MichelZ
Activate WeakRefTestYukonSpecific, it runs fine on Linux
86bcc0c
@MichelZ
Activate IntegratedAuthenticationTest, it runs fine
6f3f107
@MichelZ
Activate LocalDBTest, make sure Local DB is present. They run fine
d614b58
@MichelZ
Activate reliability tests, they run fine
c810732
@MichelZ
Activate Credential Tests, they seem to work fine
8ca44b9
@MichelZ
Activate CommandCancel tests, add Conditions to existing issue
f854d0e
@MichelZ
Activate Udt tests, they seem to work fine
241a241
@MichelZ
Activate PacketNumberWraparound test, it works fine
f1765a2
@MichelZ
Improve certificate tests by checking IsAdmin outside the constructor
a0844a8
@MichelZ
- Remove AsyncTest (why would Async be faster than Sync?)
8a83e20 
- Move tests around to TestSet when necessary, consolidate AE test sets
@codecov Codecov
Copy link

codecov bot commented Nov 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.95%. Comparing base (46e8714) to head (1034f92).
Report is 5 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3012      +/-   ##
==========================================
+ Coverage   72.66%   72.95%   +0.29%     
==========================================
  Files         283      283              
  Lines       58975    58975              
==========================================
+ Hits        42853    43027     +174     
+ Misses      16122    15948     -174
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.79% <ø> (+0.35%) ⬆️
netfx 71.18% <ø> (+0.05%) ⬆️

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
Copy link
Contributor

ErikEJ commented Nov 18, 2024

@MichelZ Maybe worth mentioning that this PR includes a product change?

edwardneal
edwardneal reviewed Nov 18, 2024
View reviewed changes
Copy link
Contributor

@edwardneal edwardneal left a comment

Choose a reason for hiding this comment

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

Thanks. The corefx issues were moved to runtime, so accessing dotnet/corefx/issues/ID will redirect appropriately. This doesn't actually help though, none of the IDs seem to correspond to issues in corefx, runtime or SqlClient.

I've added a few comments, but I can't validate whether any of the ActiveIssue removals are valid because I can't tell where to look. Where we need to retain the ActiveIssue attributes, I'd suggest using this PR to set & enforce a code standard where identifiers in these attribute include a reference to the bug tracking system in question. The runtime repo uses the issue URLs, which seems like a sensible starting point...

...crosoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/AsyncTest.cs Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionTestWithSSLCert/CertificateTest.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ExceptionTest/ConnectionExceptionTest.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionStringBuilder.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlExceptionTest.cs Outdated Show resolved Hide resolved
@MichelZ @edwardneal
Fix spelling
4c0448b 
Co-authored-by: Edward Neal <[email protected]>
@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 18, 2024

Thanks. The corefx issues were moved to runtime, so accessing dotnet/corefx/issues/ID will redirect appropriately. This doesn't actually help though, none of the IDs seem to correspond to issues in corefx, runtime or SqlClient.

I've added a few comments, but I can't validate whether any of the ActiveIssue removals are valid because I can't tell where to look. Where we need to retain the ActiveIssue attributes, I'd suggest using this PR to set & enforce a code standard where identifiers in these attribute include a reference to the bug tracking system in question. The runtime repo uses the issue URLs, which seems like a sensible starting point...

Yes, but I have not found any "valid" issue ID's (I have checked a few)

@David-Engel
Copy link
Contributor

The issue IDs correspond to issues filed in our internal AzDO system. They are still valid, just not visible publicly. Interesting that the serialization issue fixed itself. 😄 The original exception from 2020 was

Error message
System.Runtime.Serialization.SerializationException : Member 'ClassName' was not found.


Stack trace
   at System.Runtime.Serialization.SerializationInfo.GetElement(String name, Type& foundType)
   at System.Runtime.Serialization.SerializationInfo.GetString(String name)
   at System.Exception..ctor(SerializationInfo info, StreamingContext context)
   at lambda_method(Closure , Object[] )
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateISerializable(JsonReader reader, JsonISerializableContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateISerializableItem(JToken token, Type type, JsonISerializableContract contract, JsonProperty member)
   at System.Runtime.Serialization.SerializationInfo.GetValue(String name, Type type)
   at System.Exception..ctor(SerializationInfo info, StreamingContext context)
   at Microsoft.Data.SqlClient.SqlException..ctor(SerializationInfo si, StreamingContext sc) in /home/dotnet/ci-agent2/_work/2/s/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlException.cs:line 36
   at lambda_method(Closure , Object[] )
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateISerializable(JsonReader reader, JsonISerializableContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonConvert.DeserializeObject(String value, Type type, JsonSerializerSettings settings)
   at Newtonsoft.Json.JsonConvert.DeserializeObject[T](String value, JsonSerializerSettings settings)
   at Microsoft.Data.SqlClient.Tests.SqlExceptionTest.SerializationTest() in /home/dotnet/ci-agent2/_work/2/s/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlExceptionTest.cs:line 19

@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 19, 2024

The issue IDs correspond to issues filed in our internal AzDO system. They are still valid, just not visible publicly.
Ah. Would it be a good idea to:

  • A) Use the full URL to the issue in AzDO so it's immediately clear they are not publicly visible
  • B) Maybe add a small comment to the Test describing the issue so we don't need the private ticket information

or my favourite:

  • C) Use a public GitHub issue as the ActiveIssue and have additional private information linked from there

@cheenamalhotra cheenamalhotra mentioned this pull request Nov 19, 2024
Open
@MichelZ MichelZ changed the title Activate "ActiveIssue" tests Tests | Activate "ActiveIssue" tests Nov 25, 2024
@mdaigle mdaigle added ➕ Code Health Issues/PRs that are targeted to source code quality improvements. Area\Tests Issues that are targeted to tests or test projects and removed ➕ Code Health Issues/PRs that are targeted to source code quality improvements. labels Nov 25, 2024
@benrr101 benrr101 mentioned this pull request Dec 2, 2024
Open
@benrr101
Copy link
Contributor

benrr101 commented Dec 2, 2024

@MichelZ can you address the concern @ErikEJ raised about this including a product change?

@MichelZ
Copy link
Contributor Author

MichelZ commented Dec 3, 2024

@benrr101 I guess @ErikEJ was talking about the change to DeprecatedKeywordsCount which was reverted here:
8fb9d0b

Only test files are touched now

@cheenamalhotra cheenamalhotra added this to the 7.0-preview1 milestone Dec 3, 2024
@MichelZ MichelZ mentioned this pull request Dec 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edwardneal edwardneal edwardneal left review comments

@ErikEJ ErikEJ ErikEJ left review comments

@benrr101 benrr101 benrr101 left review comments

@mdaigle mdaigle Awaiting requested review from mdaigle

@apoorvdeshmukh apoorvdeshmukh Awaiting requested review from apoorvdeshmukh

@cheenamalhotra cheenamalhotra Awaiting requested review from cheenamalhotra

@samsharma2700 samsharma2700 Awaiting requested review from samsharma2700

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Area\Tests Issues that are targeted to tests or test projects
Projects
None yet
Milestone
7.0-preview1
Development

Successfully merging this pull request may close these issues.

Fix XmlReader Test on ARM
7 participants
@MichelZ @ErikEJ @David-Engel @benrr101 @edwardneal @mdaigle @cheenamalhotra