Enable Nullable Reference Types in Test Projects #44862

v-wuzhai · 2024-11-14T05:56:41Z

This PR addresses enabling nullable reference types for the following test projects:

Microsoft.Win32.Msi.Manual.Tests.csproj
HelixTasks.csproj
Microsoft.NET.TestFramework.csproj

marcpopMSFT · 2024-11-15T00:16:04Z

test/HelixTasks/SDKCustomCreateXUnitWorkItemsWithTestExclusion.cs

@@ -22,7 +22,7 @@ public class SDKCustomCreateXUnitWorkItemsWithTestExclusion : Build.Utilities.Ta
        /// The two required parameters will be automatically created if XUnitProject.Identity is set to the path of the XUnit csproj file
        /// </summary>
        [Required]
-        public ITaskItem[] XUnitProjects { get; set; }
+        public ITaskItem[]? XUnitProjects { get; set; }


@rainersigwald Have ya'll enabled nullable for any msbuild tasks? Is it possible for these to be null or does MSBuild force them to have a value always? Do they have to be marked Required to not be null?

marcpopMSFT · 2024-11-15T00:18:05Z

test/Common/Program.cs

@@ -66,7 +66,7 @@ private static int ShowSdkInfo()
    {
        var log = new StringTestLogger();
        var command = new DotnetCommand(log, "--info");
-        var testDirectory = TestDirectory.Create(Path.Combine(TestContext.Current.TestExecutionDirectory, "sdkinfo"));
+        var testDirectory = TestDirectory.Create(Path.Combine(TestContext.Current?.TestExecutionDirectory!, "sdkinfo"));


I wonder if rather than having the ! here we should have the property getter for TestExecutionDirectory throw if it's null (as it really should never be null). That would potentially remove a lot of these !s that you've added by doing it in one place.

… null safety based on PR feedback

MiYanni · 2024-12-06T02:16:33Z

@v-wuzhai I do plan on reviewing this PR at some point. However, we're having internal discussions about nullability as part of my PR. Take a look at it if you're interested.

…i/enable-nullable

v-wuzhai · 2024-12-13T07:42:01Z

@MiYanni @marcpopMSFT would you be able to review these changes? Thanks!

MiYanni

Overall, looks good. Just a few adjustments to be made based on the comments I left. Otherwise, good to go.

MiYanni · 2024-12-13T22:51:53Z

test/HelixTasks/TarGzFileCreateFromDirectory.cs


            if (!Directory.Exists(SourceDirectory))
            {
-                Log.LogError($"SourceDirectory '{SourceDirectory} does not exist.");
+                Log.LogError($"SourceDirectory '{SourceDirectory}' does not exist.");


MiYanni · 2024-12-13T22:52:04Z

test/HelixTasks/TarGzFileCreateFromDirectory.cs

@@ -33,7 +33,7 @@ public sealed class TarGzFileCreateFromDirectory : ToolTask
        /// <summary>
        /// An item group of regular expressions for content to exclude from the archive.
        /// </summary>
-        public ITaskItem[] ExcludePatterns { get; set; }
+        public ITaskItem[]? ExcludePatterns { get; set; } 


nit: trailing space

MiYanni · 2024-12-13T23:01:38Z

test/Microsoft.NET.TestFramework/Commands/AddReferenceCommand.cs

@@ -17,7 +17,7 @@ public override CommandResult Execute(IEnumerable<string> args)
        {
            List<string> newArgs = new();
            newArgs.Add("add");
-            if (!string.IsNullOrEmpty(_projectName))
+            if (_projectName is not null && !string.IsNullOrEmpty(_projectName))


We've updated the build to no longer warn for the .NET Framework compilation for string.IsNullOrEmpty so this added check is no longer necessary. See: #45299

MiYanni · 2024-12-13T23:02:14Z

test/Microsoft.NET.TestFramework/Commands/DotnetPublishCommand.cs

@@ -16,7 +16,7 @@ public DotnetPublishCommand(ITestOutputHelper log, params string[] args) : base(
        protected override SdkCommandSpec CreateCommand(IEnumerable<string> args)
        {
            List<string> newArgs = new(args);
-            if (!string.IsNullOrEmpty(_runtime))
+            if (_runtime is not null && !string.IsNullOrEmpty(_runtime))


No longer necessary.

MiYanni · 2024-12-13T23:02:48Z

test/Microsoft.NET.TestFramework/Commands/ListPackageCommand.cs

@@ -17,7 +17,7 @@ public override CommandResult Execute(IEnumerable<string> args)
        {
            List<string> newArgs = new();
            newArgs.Add("list");
-            if (!string.IsNullOrEmpty(_projectName))
+            if (_projectName is not null && !string.IsNullOrEmpty(_projectName))


No longer necessary.

MiYanni · 2024-12-13T23:03:35Z

test/Microsoft.NET.TestFramework/Commands/RemoveReferenceCommand.cs

@@ -17,7 +17,7 @@ public override CommandResult Execute(IEnumerable<string> args)
        {
            List<string> newArgs = new();
            newArgs.Add("remove");
-            if (!string.IsNullOrEmpty(_projectName))
+            if (_projectName is not null && !string.IsNullOrEmpty(_projectName))


No longer necessary.

MiYanni · 2024-12-13T23:19:35Z

test/Microsoft.NET.TestFramework/ProjectConstruction/TestProject.cs

-            if (ProjectSdk != null)
+            if (projectXml.Root.Attribute("Sdk") is not null && ProjectSdk != null)
            {
-                projectXml.Root.Attribute("Sdk").Value = ProjectSdk;
+                projectXml.Root.Attribute("Sdk")!.Value = ProjectSdk;
            }


Might be better to do something like this. Not sure it will work exactly like this since I'm not sure of the types used here. Might need to play around with it.

Suggested change

if (ProjectSdk != null)

if (projectXml.Root.Attribute("Sdk") is not null && ProjectSdk != null)

{

projectXml.Root.Attribute("Sdk").Value = ProjectSdk;

projectXml.Root.Attribute("Sdk")!.Value = ProjectSdk;

}

var sdkAttribute = projectXml.Root.Attribute("Sdk");

if (sdkAttribute is not null && ProjectSdk != null)

{

sdkAttribute.Value = ProjectSdk;

}

MiYanni · 2024-12-13T23:29:35Z

test/Microsoft.NET.TestFramework/TestAsset.cs

+                        var packageReferencesToUpdate =
+                            project.Root.Descendants(ns + PropertyName)
+                                .Where(p => p.Attribute("Version") != null && p.Attribute("Version")!.Value.Equals($"$({targetName})", StringComparison.OrdinalIgnoreCase));
+                        foreach (var packageReference in packageReferencesToUpdate)
+                        {
+                            if(packageReference.Attribute("Version") is not null)
+                            {
+                                packageReference.Attribute("Version")!.Value = targetValue;
+                            }
+                        }


Similar to the comment above. You might need to set the type on versionAttributesToUpdate to be explicitly not null for it to resolve in the foreach properly. This avoids checking it is not null twice, since you already filter out the nulls in the Where.

Suggested change

var packageReferencesToUpdate =

project.Root.Descendants(ns + PropertyName)

.Where(p => p.Attribute("Version") != null && p.Attribute("Version")!.Value.Equals($"$({targetName})", StringComparison.OrdinalIgnoreCase));

foreach (var packageReference in packageReferencesToUpdate)

{

if(packageReference.Attribute("Version") is not null)

{

packageReference.Attribute("Version")!.Value = targetValue;

}

}

var versionAttributesToUpdate =

project.Root.Descendants(ns + PropertyName)

.Select(p => p.Attribute("Version"))

.Where(va => va is not null && va.Value.Equals($"$({targetName})", StringComparison.OrdinalIgnoreCase));

foreach (var versionAttribute in versionAttributesToUpdate)

{

versionAttribute.Value = targetValue;

}

MiYanni · 2024-12-13T23:30:15Z

test/Microsoft.NET.TestFramework/TestAssetsManager.cs

@@ -110,7 +110,7 @@ public TestAsset CreateTestProjects(

            foreach (var testProject in testProjects)
            {
-                new DotnetCommand(Log, "sln", "add", testProject.Name)
+                new DotnetCommand(Log, "sln", "add", testProject.Name?? string.Empty)


nit: missing space

MiYanni · 2024-12-13T23:46:25Z

test/Microsoft.NET.TestFramework/ToolsetInfo.cs

@@ -185,7 +195,7 @@ public void AddTestEnvironmentVariables(IDictionary<string, string> environment)
                environment.Add("DOTNET_ROOT(x86)", DotNetRoot);
            }

-            if (!string.IsNullOrEmpty(CliHomePath))
+            if (CliHomePath is not null && !string.IsNullOrEmpty(CliHomePath))


No longer necessary.

…i/enable-nullable

v-wuzhai · 2024-12-16T08:33:46Z

/azp run dotnet-sdk-public-ci,sdk-source-build,sdk-unified-build

azure-pipelines · 2024-12-16T08:34:10Z

Azure Pipelines successfully started running 3 pipeline(s).

Enable Nullable Reference Types in Test Projects

1a475e9

v-wuzhai requested review from a team and tmat as code owners November 14, 2024 05:56

dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Nov 14, 2024

v-wuzhai requested review from marcpopMSFT and MiYanni November 14, 2024 07:02

marcpopMSFT reviewed Nov 15, 2024

View reviewed changes

v-wuzhai force-pushed the dev/v-wuzhai/enable-nullable branch 2 times, most recently from 49f596d to 1676070 Compare November 20, 2024 07:48

v-wuzhai requested review from a team, AntonLapounov and vijayrkn as code owners November 20, 2024 07:48

v-wuzhai force-pushed the dev/v-wuzhai/enable-nullable branch from 1676070 to 1a475e9 Compare November 20, 2024 07:54

Refactor Current, TestExecutionDirectory, and TestAssetsDirectory for…

5e233e4

… null safety based on PR feedback

v-wuzhai added 2 commits December 8, 2024 22:51

Refactor code to remove '!' operators

596d00c

Remove '!' operators

12ee199

v-wuzhai force-pushed the dev/v-wuzhai/enable-nullable branch from 5d498be to 8e1c5db Compare December 10, 2024 05:24

Merge branch 'main' of https://github.com/dotnet/sdk into dev/v-wuzha…

8b0dee8

…i/enable-nullable

Fix build failed due to enabling nullable

2709f62

v-wuzhai force-pushed the dev/v-wuzhai/enable-nullable branch from 9f34fd2 to 2709f62 Compare December 13, 2024 06:25

dotnet deleted a comment from azure-pipelines bot Dec 13, 2024

MiYanni approved these changes Dec 13, 2024

View reviewed changes

v-wuzhai added 2 commits December 15, 2024 22:14

Update the code based on the feedback

73049a7

Merge branch 'main' of https://github.com/dotnet/sdk into dev/v-wuzha…

c982008

…i/enable-nullable

build-analysis bot mentioned this pull request Dec 16, 2024

The hosted runner encountered an error while running your job. (Error Type: Disconnect). dotnet/dnceng#1919

Open

3 tasks

v-wuzhai enabled auto-merge December 16, 2024 10:25

v-wuzhai merged commit 14bb58e into main Dec 16, 2024
38 checks passed

v-wuzhai deleted the dev/v-wuzhai/enable-nullable branch December 16, 2024 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Nullable Reference Types in Test Projects #44862

Enable Nullable Reference Types in Test Projects #44862

v-wuzhai commented Nov 14, 2024

marcpopMSFT Nov 15, 2024

marcpopMSFT Nov 15, 2024

MiYanni commented Dec 6, 2024

v-wuzhai commented Dec 13, 2024

MiYanni left a comment

MiYanni Dec 13, 2024

MiYanni Dec 13, 2024

MiYanni Dec 13, 2024

MiYanni Dec 13, 2024

MiYanni Dec 13, 2024

MiYanni Dec 13, 2024

MiYanni Dec 13, 2024

MiYanni Dec 13, 2024

MiYanni Dec 13, 2024

MiYanni Dec 13, 2024

v-wuzhai commented Dec 16, 2024

azure-pipelines bot commented Dec 16, 2024

Enable Nullable Reference Types in Test Projects #44862

Enable Nullable Reference Types in Test Projects #44862

Conversation

v-wuzhai commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiYanni commented Dec 6, 2024

v-wuzhai commented Dec 13, 2024

MiYanni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v-wuzhai commented Dec 16, 2024

azure-pipelines bot commented Dec 16, 2024