dotnet / dotnet/arcade

ConditionalClassAttribute throws in cases where ConditionalFact/ConditionalTheory do not throw

Open
#7,439 5 comments 0 reactions 0 assignees View on GitHub
help wanted
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.