Enable nullables on TestHostProvider #3738

Evangelink · 2022-06-10T07:45:04Z

Contributes to AB#1549371

Evangelink · 2022-06-10T07:46:45Z

src/Microsoft.TestPlatform.PlatformAbstractions/Friends.cs

+using System.Runtime.CompilerServices;
+
+#region Product Assemblies
+[assembly: InternalsVisibleTo("Microsoft.TestPlatform.TestHostRuntimeProvider, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]


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 · 2022-06-10T07:47:27Z

...crosoft.TestPlatform.PlatformAbstractions/Microsoft.TestPlatform.PlatformAbstractions.csproj

    <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>


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

Evangelink · 2022-06-10T07:47:52Z

src/Microsoft.TestPlatform.PlatformAbstractions/PublicAPI/PublicAPI.Shipped.txt

 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


There are cases where we pass null.

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcDumpDumper.cs

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/WindowsHangDumper.cs

cvpoienaru · 2022-06-10T09:19:01Z

src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs

 using System.IO;
 using System.Reflection;
 using System.Threading;
+#if !NET5_0_OR_GREATER


Why is this directive necessary ?

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

cvpoienaru · 2022-06-10T09:34:57Z

src/Microsoft.TestPlatform.TestHostProvider/Hosting/AppxManifestFile.cs

-                Element(ns + "Applications").
-                Element(ns + "Application").
-                Attribute("Executable").Value;
+            return doc.Element(ns + "Package")!.


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

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.

cvpoienaru · 2022-06-10T09:36:27Z

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs

-    private Process _testHostProcess;
-    private StringBuilder _testHostProcessStdError;
-    private IMessageLogger _messageLogger;
+    private Architecture _architecture;


Should _architecture be nullable too ?

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.

cvpoienaru · 2022-06-10T09:41:47Z

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs

-            Element(ns + "ItemGroup").
-            Element(ns + "AppXManifest").
-            Attribute("Include").Value;
+        string appxManifestPath = doc.Element(ns + "Project")!.


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

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).

src/Microsoft.TestPlatform.PlatformAbstractions/common/Tracing/RollingFileTraceListener.cs

Evangelink requested review from MarcoRossignoli and nohwnd as code owners June 10, 2022 07:45

Evangelink commented Jun 10, 2022

View reviewed changes

Evangelink force-pushed the hostprovider-nullable branch from 740adec to 45e862c Compare June 10, 2022 08:48

cvpoienaru reviewed Jun 10, 2022

View reviewed changes

Evangelink commented Jun 10, 2022

View reviewed changes

src/Microsoft.TestPlatform.PlatformAbstractions/common/Tracing/RollingFileTraceListener.cs Outdated Show resolved Hide resolved

Evangelink commented Jun 10, 2022

View reviewed changes

src/Microsoft.TestPlatform.PlatformAbstractions/common/Tracing/RollingFileTraceListener.cs Outdated Show resolved Hide resolved

Evangelink enabled auto-merge (squash) June 10, 2022 15:45

cvpoienaru approved these changes Jun 13, 2022

View reviewed changes

Evangelink and others added 4 commits June 13, 2022 11:57

Enable nullables on TestHostProvider

590d094

Fix missed nullability

e8dc14c

Apply suggestions from code review

b5897a4

Add placeholder for null data on OutputReceivedCallbacks

ce8adfc

Evangelink force-pushed the hostprovider-nullable branch from d99d5c8 to ce8adfc Compare June 13, 2022 10:00

MarcoRossignoli approved these changes Jun 13, 2022

View reviewed changes

Evangelink merged commit fd9d907 into microsoft:main Jun 13, 2022

Evangelink deleted the hostprovider-nullable branch June 13, 2022 15:12

This was referenced Jul 21, 2022

Upgrade Newtonsoft to v13.0.1 microsoft/sarif-pattern-matcher#627

Merged

Bump SARIF sdk and Newtonsoft Changes microsoft/binskim#674

Merged

Enable nullables on TestHostProvider #3738

Enable nullables on TestHostProvider #3738

Uh oh!

Conversation

Evangelink commented Jun 10, 2022 • edited by testplatform-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Evangelink commented Jun 10, 2022 •

edited by testplatform-bot

Loading