Target framework checks for System.Security.Crypto.Pkcs.Tests #118104
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'"> |
Copilot uses AI. Check for mistakes.
| <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.
Copilot uses AI. Check for mistakes.
|
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