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

Enable nullables on TestHostProvider #3738

Merged
merged 4 commits into from
Jun 13, 2022
Merged

Enable nullables on TestHostProvider #3738

merged 4 commits into from
Jun 13, 2022

Conversation

Evangelink
Copy link
Member

@Evangelink Evangelink commented Jun 10, 2022 •
edited by testplatform-bot
Loading

Contributes to AB#1549371

Evangelink
Evangelink commented Jun 10, 2022
View reviewed changes
src/Microsoft.TestPlatform.PlatformAbstractions/Friends.cs
using System.Runtime.CompilerServices;

#region Product Assemblies
[assembly: InternalsVisibleTo("Microsoft.TestPlatform.TestHostRuntimeProvider, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]
Copy link
Member Author

Choose a reason for hiding this comment

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

Makes PlatformAbstractions internals visible to TestHostRuntimeProvider to allow access to NullableAttributes. Adding a link to NullableAttributes directly from TestHostRuntimeProvider causes ambiguity as it gets resolved from different paths.

Evangelink
Evangelink commented Jun 10, 2022
View reviewed changes
...crosoft.TestPlatform.PlatformAbstractions/Microsoft.TestPlatform.PlatformAbstractions.csproj
@@ -7,7 +7,7 @@
<PropertyGroup>
<AssemblyName>Microsoft.TestPlatform.PlatformAbstractions</AssemblyName>
<RootNamespace>Microsoft.TestPlatform.PlatformAbstractions</RootNamespace>
<TargetFrameworks>net45;net451;netcoreapp1.0;netcoreapp2.1;netstandard1.3;netstandard2.0</TargetFrameworks>
<TargetFrameworks>net45;net451;netcoreapp1.0;netcoreapp2.1;netstandard1.3;netstandard2.0;net6.0</TargetFrameworks>
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding net6.0 because otherwise some tests fails as Abstraction is targeting netcoreapp2.1 when used in net6.0 tests projects.

Evangelink
Evangelink commented Jun 10, 2022
View reviewed changes
src/Microsoft.TestPlatform.PlatformAbstractions/PublicAPI/PublicAPI.Shipped.txt
@@ -25,8 +25,8 @@ Microsoft.VisualStudio.TestPlatform.ObjectModel.PlatformTraceLevel.Off = 0 -> Mi
Microsoft.VisualStudio.TestPlatform.ObjectModel.PlatformTraceLevel.Verbose = 4 -> Microsoft.VisualStudio.TestPlatform.ObjectModel.PlatformTraceLevel
Microsoft.VisualStudio.TestPlatform.ObjectModel.PlatformTraceLevel.Warning = 2 -> Microsoft.VisualStudio.TestPlatform.ObjectModel.PlatformTraceLevel
Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces.AssemblyResolveEventArgs
Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces.AssemblyResolveEventArgs.AssemblyResolveEventArgs(string! name) -> void
Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces.AssemblyResolveEventArgs.Name.get -> string!
Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces.AssemblyResolveEventArgs.AssemblyResolveEventArgs(string? name) -> void
Copy link
Member Author

Choose a reason for hiding this comment

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

There are cases where we pass null.

cvpoienaru
cvpoienaru reviewed Jun 10, 2022
View reviewed changes
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcDumpDumper.cs Show resolved Hide resolved
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/WindowsHangDumper.cs Show resolved Hide resolved
src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs
@@ -9,7 +9,9 @@
using System.IO;
using System.Reflection;
using System.Threading;
#if !NET5_0_OR_GREATER
Copy link
Member

Choose a reason for hiding this comment

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

Why is this directive necessary ?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a warning (unused directive) already present in the code but for some reason it was not triggered before. Enabling .NET 6.0 made it raise so I am fixing it.

src/Microsoft.TestPlatform.PlatformAbstractions/common/Tracing/RollingFileTraceListener.cs Outdated Show resolved Hide resolved
src/Microsoft.TestPlatform.PlatformAbstractions/common/Tracing/RollingFileTraceListener.cs Outdated Show resolved Hide resolved
src/Microsoft.TestPlatform.TestHostProvider/Hosting/AppxManifestFile.cs
Element(ns + "Applications").
Element(ns + "Application").
Attribute("Executable").Value;
return doc.Element(ns + "Package")!.
Copy link
Member

Choose a reason for hiding this comment

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

Are we completely sure the xml is well structured and none of those elements are missing ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Other reply gives more details but short answer is no, I am not sure but I am preserving the current in-place behavior. I have already faced a bug where changing nullability cause a bug because we were relying on a NRE caught somewhere else as part of the logic.

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs
private Process _testHostProcess;
private StringBuilder _testHostProcessStdError;
private IMessageLogger _messageLogger;
private Architecture _architecture;
Copy link
Member

Choose a reason for hiding this comment

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

Should _architecture be nullable too ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could make it nullable but it's a struct and current default behavior is not null. Code is making lots of assumptions everywhere so it's not clear whether we are supposed to always have been through Initialize before using this one.

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs
Element(ns + "ItemGroup").
Element(ns + "AppXManifest").
Attribute("Include").Value;
string appxManifestPath = doc.Element(ns + "Project")!.
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure the xml is well structured and none of the elements are missing ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not but I could not find anything guiding me in one way or another so I have preserved in place behavior (i.e. assuming it's not-null with the risk of failing).

@Evangelink Evangelink enabled auto-merge (squash) June 10, 2022 15:45
@Evangelink Evangelink merged commit fd9d907 into microsoft:main Jun 13, 2022
@Evangelink Evangelink deleted the hostprovider-nullable branch June 13, 2022 15:12
This was referenced Jul 21, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcoRossignoli MarcoRossignoli MarcoRossignoli approved these changes

@cvpoienaru cvpoienaru cvpoienaru approved these changes

@nohwnd nohwnd Awaiting requested review from nohwnd nohwnd is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Evangelink @cvpoienaru @MarcoRossignoli