Target framework checks for System.Security.Crypto.Pkcs.Tests#118104
Target framework checks for System.Security.Crypto.Pkcs.Tests#118104PranavSenthilnathan wants to merge 1 commit intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces target framework-aware helpers for System.Security.Cryptography.Pkcs tests to replace runtime platform detection with compile-time target framework checks. The change addresses issue #118103 by ensuring tests behave correctly based on the target framework rather than the runtime platform.
Key Changes:
- Introduces
TargetFrameworkHelpersclass with platform-specific implementations - Replaces
PlatformDetection.IsWindowscalls withTargetFrameworkHelpers.TargetsWindowsin test files - Configures conditional compilation based on target framework and platform identifiers
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| TargetFrameworkHelpers.Windows.cs | Windows-specific implementation returning true for TargetsWindows |
| TargetFrameworkHelpers.AnyOS.cs | Non-Windows implementation returning false for TargetsWindows |
| System.Security.Cryptography.Pkcs.Tests.csproj | Conditional compilation configuration for helper classes |
| KeyTransRecipientInfoRsaPaddingModeTests.cs | Updates platform check for RSA OAEP certificate support |
| KeyTransRecipientInfoRsaOaepCertTests.cs | Updates platform check for RSA OAEP certificate support |
| KeyAgreeRecipientInfoTests.cs | Updates platform check for Diffie-Hellman support |
| GeneralTests.cs | Updates platform check for RSA OAEP certificate support |
| ContentEncryptionAlgorithmTests.cs | Updates platform check for RC4 support |
| <Compile Include="Pkcs8PrivateKeyInfoTests.cs" /> | ||
| <Compile Include="PrivateKeyHelpers.cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' or '$(TargetPlatformIdentifier)' == 'windows'"> |
There was a problem hiding this comment.
The condition should use lowercase 'windows' for consistency with MSBuild conventions. Consider using '$(TargetPlatformIdentifier)' == 'Windows' with proper casing, or verify that the lowercase 'windows' is intentional and documented.
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' or '$(TargetPlatformIdentifier)' == 'windows'"> | |
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' or '$(TargetPlatformIdentifier)' == 'Windows'"> |
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' or '$(TargetPlatformIdentifier)' == 'windows'"> | ||
| <Compile Include="TargetFrameworkHelpers.Windows.cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework' and '$(TargetPlatformIdentifier)' != 'windows'"> |
There was a problem hiding this comment.
Same casing issue as above - the condition should use consistent casing for 'windows'. This condition is the logical inverse of line 79, so both should use the same casing convention.
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
| { | ||
| public static bool SupportsRc2 => PlatformSupport.IsRC2Supported; | ||
| public static bool SupportsRc4 => PlatformDetection.IsWindows; | ||
| public static bool SupportsRc4 => TargetFrameworkHelpers.TargetsWindows; |
There was a problem hiding this comment.
I don't think we should take this change, at all.
A change which makes the VS runner see the same assets as the CLI runner, sure. But random extra hacking on the side, no. It will just leave behind a continual specter of "should this use TargetsWindows, or IsWindows?".
|
Any change should only be in the project file or config for test discoverer/runner instead of in our test code. |
Fixes #118103