You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
ConditionalClassAttribute throws in cases where ConditionalFact/ConditionalTheory do not throw #7439
Last week, @jozkee and I I worked on this PR and this PR 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 and FileSystemTest (which then calls MountHelper).
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 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.
@jozkeedecided to fix 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?
I think this is one of those things that got dumped in arcade, but no one in dnceng really owns. Sort of like RemoteExecutor. I'm not sure what the current policy is for things there arcade is the dumping ground for.
We own this library an do not expect core-eng to follow up on issues. Should we add a code owners entry for so that we at least get notified of prs targeting the code?
Regarding the issue, will take a look. cc @akoeplinger
Activity
MattGal commentedon May 27, 2021
[Async Triage]: Not FR as it has a workaround, perhaps some arcade technical debt epic.
MattGal commentedon Jun 3, 2021
[Async Triage]: No change, pinged the team to see if I should stop doing this.
ChadNedzlek commentedon Jun 3, 2021
I think this is one of those things that got dumped in arcade, but no one in dnceng really owns. Sort of like RemoteExecutor. I'm not sure what the current policy is for things there arcade is the dumping ground for.
ViktorHofer commentedon Jun 3, 2021
We own this library an do not expect core-eng to follow up on issues. Should we add a code owners entry for so that we at least get notified of prs targeting the code?
Regarding the issue, will take a look. cc @akoeplinger