Replace mono with .NET 8 support #120
Conversation
| @@ -29,7 +29,7 @@ public void TestArgumentValidation([Values] bool isWindowsSyntax) | |||
| [TestCase("\r", "\n", "\r\n")] | |||
| [TestCase("", "\"", "\\", "")] | |||
| [TestCase("abc", "a\\b", "a\\ b\"")] | |||
| // these chars are treated specially on mono unix | |||
| // these chars are treated specially on mono unix, so keeping. | |||
There was a problem hiding this comment.
Whenever I wasn't sure what to do about Mono-related comments, I either deleted it or modified it so that it shows up in the diff.
| public class PlatformCompatibilityTest | ||
| { | ||
| [Test] | ||
| public Task TestReadAfterExit() => RunTestAsync(() => PlatformCompatibilityTests.TestReadAfterExit()); | ||
|
|
||
| [Test] | ||
| public Task TestWriteAfterExit() => RunTestAsync(() => PlatformCompatibilityTests.TestWriteAfterExit()); | ||
| // TODO: fix in https://github.com/madelson/MedallionShell/issues/117 |
There was a problem hiding this comment.
I think this is fine for now because this must be fixed before 1.7 is released.
| // other types of IOExceptions (e. g. FileNotFoundException, PathTooLongException) that could in | ||
| // theory be thrown here and trigger this | ||
| when (IsMono && ex.GetType() == typeof(IOException)) | ||
| catch |
There was a problem hiding this comment.
Also not sure what to do here...
| @@ -114,8 +111,8 @@ public static void TestBadProcessFile() | |||
| { | |||
| var baseDirectory = Path.GetDirectoryName(SampleCommandPath)!; | |||
|
|
|||
| AssertThrows<Win32Exception>(() => Command.Run(baseDirectory)); | |||
| AssertThrows<Win32Exception>(() => Command.Run(Path.Combine(baseDirectory, "DOES_NOT_EXIST.exe"))); | |||
| AssertThrows<InvalidOperationException>(() => Command.Run(baseDirectory)); | |||
There was a problem hiding this comment.
This was failing for all target frameworks, so I think this is a breaking change in a more recent version of .NET SDK, but I couldn't find anything that seems relevant in https://learn.microsoft.com/en-us/dotnet/core/compatibility/8.0 or https://learn.microsoft.com/en-us/dotnet/core/compatibility/7.0.
| @@ -22,13 +22,8 @@ for: | |||
| matrix: | |||
| only: | |||
| - | |||
| image: "Ubuntu" | |||
There was a problem hiding this comment.
Missed in the previous PR; we temporarily lost coverage for Ubuntu 22.04.
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="'$(TargetFramework)' == 'net462'"> | ||
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'"> |
There was a problem hiding this comment.
This way, we don't have to update this line when we change from net462 to net472. You can see that this technique is being used in https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/dev/src/Microsoft.IdentityModel.Protocols/Microsoft.IdentityModel.Protocols.csproj#L29.
|
@madelson This blocks other PRs, so I'd appreciate it if you could review this first! |
@madelson There are some open questions in #119, but filing a PR to get some feedback.