[tests] Stop disabling nullability.#25044
[tests] Stop disabling nullability.#25044rolfbjarne wants to merge 1 commit intodev/rolf/src-nullability-miscfrom
Conversation
And fix any resulting issues.
There was a problem hiding this comment.
Pull request overview
This PR removes #nullable disable from various test projects and updates the affected code to compile under nullable reference types, aligning the test suite with the repo’s nullable expectations.
Changes:
- Enabled
#nullable enableacross multiple introspection and common test files. - Updated test helpers and reflection-heavy tests with nullable annotations and null-handling adjustments.
- Added
System.Diagnostics.CodeAnalysisglobal using to support nullability attributes in tests.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rgen/Microsoft.Macios.Transformer.Tests/BaseTransformerTestClass.cs | Updates transformer test compilation setup for nullable config values. |
| tests/rgen/Microsoft.Macios.Generator.Tests/BaseGeneratorTestClass.cs | Updates generator test compilation setup for nullable config values. |
| tests/introspection/iOSApiWeakPropertyTest.cs | Enables nullable and adjusts reflection usage. |
| tests/introspection/iOSApiSignatureTest.cs | Enables nullable and updates reflection nullability. |
| tests/introspection/iOSApiSelectorTest.cs | Enables nullable and updates DeclaringType handling. |
| tests/introspection/iOSApiFieldTest.cs | Enables nullable and adjusts reflection namespace access. |
| tests/introspection/MacApiPInvokeTest.cs | Aligns override signature with nullable base method. |
| tests/introspection/CoreSelectorTest.cs | Enables nullable and adjusts DeclaringType usage. |
| tests/introspection/ApiWeakPropertyTest.cs | Enables nullable and updates out-parameter nullability patterns. |
| tests/introspection/ApiTypoTest.cs | Enables nullable and updates helper method signatures. |
| tests/introspection/ApiTypeTest.cs | Enables nullable and updates type name handling. |
| tests/introspection/ApiSignatureTest.cs | Enables nullable and updates signature parsing state to nullable. |
| tests/introspection/ApiSelectorTest.cs | Enables nullable and adjusts protocol/member inspection helpers. |
| tests/introspection/ApiPInvokeTest.cs | Enables nullable and updates P/Invoke inspection nullability. |
| tests/introspection/ApiFrameworkTest.cs | Enables nullable and adjusts namespace extraction. |
| tests/introspection/ApiFieldTest.cs | Enables nullable and updates reflection accessor nullability. |
| tests/introspection/ApiCtorInitTest.cs | Enables nullable and updates ctor/reflection and exception handling annotations. |
| tests/introspection/ApiCoreImageFiltersTest.cs | Enables nullable and updates filter attribute handling. |
| tests/introspection/ApiClassPtrTest.cs | Enables nullable and updates extension-type detection return type. |
| tests/introspection/ApiCMAttachmentTest.cs | Enables nullable and updates instance-creation helpers and casts. |
| tests/introspection/ApiBaseTest.cs | Enables nullable and adjusts helpers’ parameter nullability and writer access. |
| tests/generator/BGenTool.cs | Enables nullable and updates config-dependent argument building. |
| tests/dotnet/UnitTests/ExtensionsTest.cs | Adjusts environment dictionary to allow nullable values. |
| tests/common/PlatformInfo.cs | Enables nullable and updates platform/version parsing types. |
| tests/common/GlobalUsings.cs | Adds System.Diagnostics.CodeAnalysis global using. |
| tests/common/ExecutionHelper.cs | Enables nullable and updates Execute overload signatures. |
| tests/common/ConfigurationNUnit.cs | Enables nullable for test configuration code. |
| tests/common/Configuration.cs | Enables nullable and annotates config variables / helper methods. |
| tests/common/AppDelegate.cs | Enables nullable and updates app runner/window nullability. |
| timed_out = rv.TimedOut; | ||
| if (rv.TimedOut) | ||
| Console.WriteLine ($"Command '{fileName} {StringUtils.FormatArguments (arguments)}' didn't finish in {timeout.Value.TotalMilliseconds} ms, and was killed.", timeout.Value.TotalMinutes); | ||
| Console.WriteLine ($"Command '{fileName} {StringUtils.FormatArguments (arguments)}' didn't finish in {timeout!.Value.TotalMilliseconds} ms, and was killed.", timeout!.Value.TotalMinutes); |
There was a problem hiding this comment.
This Console.WriteLine call passes an extra argument (timeout!.Value.TotalMinutes) that isn’t used by the interpolated string, and it also null-forgives timeout. Either include the value in the message (add a placeholder/text) or remove the extra argument, and avoid timeout! by handling the timeout is null case.
| timed_out = rv.TimedOut; | ||
| if (rv.TimedOut) | ||
| Console.WriteLine ($"Command '{fileName} {StringUtils.FormatArguments (arguments)}' didn't finish in {timeout.Value.TotalMilliseconds} ms, and was killed.", timeout.Value.TotalMinutes); | ||
| Console.WriteLine ($"Command '{fileName} {StringUtils.FormatArguments (arguments)}' didn't finish in {timeout!.Value.TotalMilliseconds} ms, and was killed.", timeout!.Value.TotalMinutes); |
There was a problem hiding this comment.
This Console.WriteLine call passes an extra argument (timeout!.Value.TotalMinutes) that isn’t used by the interpolated string, and it also null-forgives timeout. Either include the value in the message (add a placeholder/text) or remove the extra argument, and avoid timeout! by handling the timeout is null case.
|
|
||
| public ApplePlatform Name { get; private set; } | ||
| public Version Version { get; private set; } | ||
| public Version? Version { get; private set; } |
There was a problem hiding this comment.
PlatformInfo.Version is now nullable, but the rest of this file compares targetPlatform.Version with other versions (using >=) and uses the result in if statements. With a nullable Version?, those comparisons become lifted to bool? and won’t compile/behave as intended. Keep Version non-nullable (initialize it to a default value) or update the comparison logic to handle null explicitly.
| public Version? Version { get; private set; } | |
| public Version Version { get; private set; } = new Version (0, 0); |
| { | ||
| // get the dotnet bcl and fully load it for the test. | ||
| var references = Directory.GetFiles (Configuration.DotNetBclDir, "*.dll") | ||
| var references = Directory.GetFiles (Configuration.DotNetBclDir!, "*.dll") |
There was a problem hiding this comment.
Configuration.DotNetBclDir is nullable, but it’s null-forgiven here. If DOTNET_BCL_DIR isn’t set in the test environment, this will throw from Directory.GetFiles with a low-signal error. Add an explicit validation and throw/Assert with a helpful message.
| var references = Directory.GetFiles (Configuration.DotNetBclDir!, "*.dll") | |
| var dotNetBclDir = Configuration.DotNetBclDir; | |
| if (string.IsNullOrEmpty (dotNetBclDir)) { | |
| throw new InvalidOperationException ("DOTNET_BCL_DIR must be set to the .NET BCL directory for generator tests to run. Configuration.DotNetBclDir was null or empty."); | |
| } | |
| var references = Directory.GetFiles (dotNetBclDir, "*.dll") |
| if (!StringUtils.TryParseArguments (Configuration.DotNetCscCommand!, out var args, out var ex)) | ||
| throw new InvalidOperationException ($"Unable to parse the .NET csc command '{Configuration.DotNetCscCommand}': {ex.Message}"); | ||
|
|
There was a problem hiding this comment.
Configuration.DotNetCscCommand is nullable, but it’s null-forgiven before calling StringUtils.TryParseArguments, whose parameter is non-nullable. If the configuration value is missing, this will throw before producing the intended error message. Validate DotNetCscCommand explicitly and fail with a clear message instead of using the null-forgiving operator.
| if (!StringUtils.TryParseArguments (Configuration.DotNetCscCommand!, out var args, out var ex)) | |
| throw new InvalidOperationException ($"Unable to parse the .NET csc command '{Configuration.DotNetCscCommand}': {ex.Message}"); | |
| var dotNetCscCommand = Configuration.DotNetCscCommand; | |
| if (dotNetCscCommand is null) | |
| throw new InvalidOperationException (".NET csc command is not configured."); | |
| if (!StringUtils.TryParseArguments (dotNetCscCommand, out var args, out var ex)) | |
| throw new InvalidOperationException ($"Unable to parse the .NET csc command '{dotNetCscCommand}': {ex.Message}"); |
| References.AddRange (Directory.GetFiles (Configuration.DotNetBclDir!, "*.dll")); | ||
| } |
There was a problem hiding this comment.
Configuration.DotNetBclDir is nullable, but it’s null-forgiven when enumerating BCL references. If it’s not set, Directory.GetFiles will throw. Prefer validating DotNetBclDir once (and giving a clear error) before using it.
| var path = FindLibrary (libname!, requiresFullPath: true); | ||
|
|
There was a problem hiding this comment.
FindLibrary (libname!, ...) can be invoked with a null libname (it’s assigned null for certain DllImport values above). This will throw at runtime. Add a guard for libname is null (skip/continue or call dlopen (null, ...)) instead of null-forgiving it.
| var path = FindLibrary (libname!, requiresFullPath: true); | |
| if (libname is null) | |
| continue; | |
| var path = FindLibrary (libname, requiresFullPath: true); |
| var fname = attributes [(NSString) "CIAttributeFilterName"]?.ToString (); | ||
| writer.WriteLine ("interface {0} {{", fname); |
There was a problem hiding this comment.
fname is now computed with a null-conditional, which can silently produce null and generate invalid output (interface {) if the attribute is missing/unexpected. Consider keeping this strict (fail fast with a clear error) or providing a non-null fallback name (e.g. from the filter/type name).
| .WithDocumentationMode (DocumentationMode.None); | ||
|
|
||
| var references = parseResult.GetReferences (workingDirectory, Configuration.DotNetBclDir).ToList (); | ||
| var references = parseResult.GetReferences (workingDirectory, Configuration.DotNetBclDir!).ToList (); |
There was a problem hiding this comment.
Configuration.DotNetBclDir is nullable, but it’s null-forgiven here. If the test configuration didn’t set DOTNET_BCL_DIR, this will fail later with an ArgumentNullException from Roslyn/IO. Add an explicit check with a clear failure message (or make the configuration value non-nullable and initialize it accordingly).
| } catch (Exception? e) { | ||
| // Objective-C exception thrown | ||
| if (!ContinueOnFailure) | ||
| throw; | ||
|
|
||
| TargetInvocationException tie = (e as TargetInvocationException); | ||
| var tie = (e as TargetInvocationException); | ||
| if (tie is not null) | ||
| e = tie.InnerException; | ||
| ReportError ("Default constructor not allowed for {0} : {1}", instance_type_name, e.Message); | ||
| ReportError ("Default constructor not allowed for {0} : {1}", instance_type_name, e?.Message); |
There was a problem hiding this comment.
The exception variable in a catch clause is never null. Using catch (Exception? e) (and then null-propagating e?.Message) is misleading and can hide real nullability issues elsewhere. Prefer catch (Exception e) and treat e as non-null.
✅ [PR Build #389ac27] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [CI Build #389ac27] Build passed (Build packages) ✅Pipeline on Agent |
✅ [CI Build #389ac27] Build passed (Build macOS tests) ✅Pipeline on Agent |
🔥 [CI Build #389ac27] Test results 🔥Test results❌ Tests failed on VSTS: test results 7 tests crashed, 0 tests failed, 131 tests passed. Failures❌ dotnettests tests (iOS)🔥 Failed catastrophically on VSTS: test results - dotnettests_ios (no summary found). Html Report (VSDrops) Download ❌ dotnettests tests (MacCatalyst)🔥 Failed catastrophically on VSTS: test results - dotnettests_maccatalyst (no summary found). Html Report (VSDrops) Download ❌ dotnettests tests (macOS)🔥 Failed catastrophically on VSTS: test results - dotnettests_macos (no summary found). Html Report (VSDrops) Download ❌ framework tests🔥 Failed catastrophically on VSTS: test results - framework (no summary found). Html Report (VSDrops) Download ❌ generator tests🔥 Failed catastrophically on VSTS: test results - generator (no summary found). Html Report (VSDrops) Download ❌ monotouch tests (iOS)🔥 Failed catastrophically on VSTS: test results - monotouch_ios (no summary found). Html Report (VSDrops) Download ❌ xcframework tests🔥 Failed catastrophically on VSTS: test results - xcframework (no summary found). Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
And fix any resulting issues.