ConditionalClassAttribute throws in cases where ConditionalFact/ConditionalTheory do not throw
- Dominant language
- C#
- Stars
- 729
- Forks
- 397
- Avg merge
- 3d 15m
- Merged PRs (30d)
- 149
Description
Last week, @Jozkee and I I worked on [this PR](https://github.com/dotnet/runtime/pull/52749) and [this PR](https://github.com/dotnet/runtime/pull/52679) in which we added unit tests that had to create symbolic links.
Some operating systems are unable to create symlinks: iOS, Android, tvOS and Browser. This is why I had to add a `Conditional*` attribute that would call the method `CanCreateSymbolicLinks`.
The `CanCreateSymbolicLinks` method tries to create a symbolic link in the current OS, and returns false if it was unable to do so.
There are two different places where we define a `CanCreateSymbolicLinks` method, but they both do exactly the same work: [FileSystemWatcherTest](https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs#L438-L487) and [FileSystemTest](https://github.com/dotnet/runtime/blob/65020c946254c9a99497f4fdfa4136131f51f913/src/libraries/System.IO.FileSystem/tests/FileSystemTest.cs#L63-L82) (which then calls [MountHelper](https://github.com/dotnet/runtime/blob/b08a97afb8270511c39fe26d37038877bb63a09b/src/libraries/System.IO.FileSystem/tests/PortedCommon/ReparsePointUtilities.cs#L34-L60)).
Initially, we both decided to add `ConditionalClass` on top of our new test classes, but our CI runs failed in the aforementioned operating systems, because `System.Diagnostics.Process` is not supported:
```
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
---> System.PlatformNotSupportedException: Operation is not supported on this platform.
at System.Diagnostics.Process.StartCore(ProcessStartInfo startInfo) in System.Diagnostics.Process.dll:token 0x6000105+0xe
at System.Diagnostics.Process.Start() in System.Diagnostics.Process.dll:token 0x60000de+0xab
at System.IO.Tests.FileSystemWatcherTest.CreateSymLink(String targetPath, String linkPath, Boolean isDirectory) in System.IO.FileSystem.Watcher.Tests.dll:token 0x6000129+0x8c
```
I decided to [fix my tests](https://github.com/dotnet/runtime/pull/52749/commits/6ef2a60c7e0c4b796bb9040ff5396b490716b57c) by changing `ConditionalClass` for `ConditionalFact` and `ConditionalTheory` (see here) and the CI passed. This tells me that `ConditionalFact` and `ConditionalTheory` are wrapped by a large try catch that returns false if an exception is thrown.
@Jozkee [decided to fix](https://github.com/dotnet/runtime/pull/52679) the `CanCreateSymbolicLinks` method he's consuming by checking the OS, and if it's unsupported, return false. While I think this fix is fine to get him unblocked, I think this is just a workaround and not a fix to the actual problem.
Since we own `ConditionalClass` in this repo, can it be fixed to also try catch all exceptions and return false?
Contributor guide
Assessment
This issue has not been assessed yet.