Add cocoa fact attribute and related unit tests #68

SiyuGithub · 2023-01-11T18:57:35Z

Add [CocoaFact] attribute to run unit tests on the Main UI threat. Before this PR, StaFact doesn't have a suitable test attribute to allow others to run their UI thread only unit tests on Mac.

What is included in this PR:
Implement new [CocoaFact] attribute. Add a MacOS app bundle which contains a Xunit built-in test runner to run new attribute unit tests on the UI Main thread.

…ntextAdapter.cs

sandyarmstrong · 2023-01-11T21:50:32Z

This is a work-in-progress branch, we'll take it out of draft when it's ready for review.

AArnott · 2023-01-13T13:51:34Z

Directory.Build.props

    <ProduceReferenceAssembly>true</ProduceReferenceAssembly>
    <EnableWindowsTargeting>true</EnableWindowsTargeting>
    <TargetingWindows Condition=" '$(TargetFramework)' == 'net472' or $(TargetFramework.EndsWith('-windows')) ">true</TargetingWindows>
+    <TargetingMacos Condition=" '$(TargetFramework)' == 'net6.0-macos10.15' or $(TargetFramework.EndsWith('-macos')) ">true</TargetingMacos>


There is a contains syntax that may be better to use here if you're liable to use -macos10.15, so that the condition doesn't silently start failing when you later move to -macos12.
I think it's roughly this:
$(TargetFramework.Contains('macos'))

AArnott · 2023-01-13T13:52:21Z

global.json

 {
  "sdk": {
-    "version": "7.0.101",
+    "version": "7.0.100",


I presume this is just to allow you to build locally on your box and not because 7.0.101 is broken somehow. We'll need to revert this before merging. You could do it now and upgrade your SDK to solve the problem if you want.

Yup, this whole branch was just us getting far enough locally to have something we could mess with in the Mac editor project. It's definitely not cleaned up or complete yet. 😄

AArnott · 2023-01-13T13:53:20Z

I don't know what the effect of .DS_Store files are on a mac, but I don't want them in the repo. Please remove them and add to the .gitignore file to ensure they don't come back.

AArnott · 2023-01-13T13:56:49Z

src/Xunit.StaFact/net472/PublicAPI.Unshipped.txt

+Xunit.Sdk.CocoaFactDiscoverer
+Xunit.Sdk.CocoaFactDiscoverer.CocoaFactDiscoverer(Xunit.Abstractions.IMessageSink! diagnosticMessageSink) -> void


Should these cocoa attributes really be exposed to net472? I get that mono used to target this, but that's history right? Can we expose the new attribute exclusively to the macos target framework?
Actually, looking at your msbuild conditions, it doesn't look like they should be exposed to net472 after all, which makes me wonder if you're getting compilation warnings suggesting you remove them.

You are right that we won't expose those to net472, I will clean them up.

AArnott · 2023-01-13T13:57:31Z

src/Xunit.StaFact/Xunit.StaFact.csproj

+  <ItemGroup>
+    <None Remove="Mac\" />
+  </ItemGroup>
+  <ItemGroup>
+    <Folder Include="Mac\" />
+  </ItemGroup>


Can we remove these items, please?

AArnott · 2023-01-13T13:58:57Z

src/Xunit.StaFact/Xunit.StaFact.csproj

  <PropertyGroup>
-    <TargetFrameworks>net472;netstandard2.0;net6.0;net6.0-windows</TargetFrameworks>
+    <TargetFrameworks>net472;netstandard2.0;net6.0;net6.0-windows;net6.0-macos10.15</TargetFrameworks>
+    <SupportedOSPlatformVersion Condition="'$(TargetingMacos)'=='true'">10.15</SupportedOSPlatformVersion>


I'm surprised this property isn't set by virtue of the TargetFramework mentioning the macos version. Are you sure this is necessary?

In theory, setting this is supposed to allow us to target net6.0-macos instead of net6.0-macos10.15, and still get net6.0-macos10.15 in the nupkg, but it wasn't working and we need to investigate. If we don't specify 10.15 somewhere, it uses the OS version of whoever does the build.

But yeah, it's not needed if we explicitly target net6.0-macos10.15.

If we don't specify 10.15 somewhere, it uses the OS version of whoever does the build.

Are you sure? I can build net6.0-macos from Windows, and that has no basis for a macos version.
Also, when I target net6.0-windows, that targets Windows 7 -- not the version of Windows that I am using.

It looks like each TFW can specify its own default OS version behavior: https://learn.microsoft.com/en-us/dotnet/standard/frameworks#os-version-in-tfms. So I would imagine the macOS workload targets are doing this. It could be that they pick macOS 13 by default not because that was the build host's OS version, but because that's the version associated with the Xcode SDK. Either way, we need to manually specify 10.15.

And I think I see my answer for SupportedOSPlatformVersion here: https://learn.microsoft.com/en-us/dotnet/standard/frameworks#support-older-os-versions. Seems that SupportedOSPlatformVersion indicates runtime version. So I should drop it anyway.

Need a full app bundle in order to start Cocoa mainloop.

This allows building on macOS 12 and easily consuming the nupkg from projects still using older workload versions.

sandyarmstrong · 2023-01-30T18:27:04Z

test/Xunit.StaFact.Tests.Mac/Xunit.StaFact.Tests.Mac.csproj

+  </ItemGroup>
+
+  <PropertyGroup Label="Globals">
+    <PartialTargetPath>$(RepoRootPath)\bin\Xunit.StaFact.Tests.Mac\$(Configuration)\$(TargetFramework)\osx-x64</PartialTargetPath>


The osx-x64 part here seems like it would fail on arm64.

If that's the case, you could do something like the following:

<HostArchitecture>$([System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture)</HostArchitecture> <HostArchitecture>$(HostArchitecture.ToLower())</HostArchitecture> <PartialTargetPath>$(RepoRootPath)\bin\Xunit.StaFact.Tests.Mac\$(Configuration)\$(TargetFramework)\osx-$(HostArchitecture)</PartialTargetPath>

SiyuGithub · 2023-01-31T21:10:37Z

Other than the Mac architecture problem described above, this PR is ready for review. Thanks!

P.S. Not sure if the problem I mentioned above has any influence, since the MacOS pipeline is using osx-x64 instead arm64, so all the tests are built and passed.

SiyuGithub · 2023-02-01T00:11:46Z

Hi @AArnott, can I have a code review on this PR when you have time?
And since I don't have the Write Access to the StaFact repo, do you want me to apply one, or you prefer someone else who has the access to merge it after the review? Thanks!

sandyarmstrong · 2023-02-01T16:26:29Z

azure-pipelines/dotnet-test-cloud.ps1

 }
+if ($IsMacOS) {
+  $frameworks += '-f','net6.0-macos'
+}


I think the changes to this file can be undone now, right?

AArnott · 2023-02-06T15:07:58Z

I can't build Xunit.StaFact.Tests.Mac on my Windows machine due to this error:

2>C:\Program Files\dotnet\sdk\7.0.102\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(1226,3): error MSB4019: The imported project "C:\Program Files\dotnet\sdk\7.0.102\Sdks\Microsoft.NET.Sdk\13.1.1024\targets\Xamarin.Shared.Sdk.MultiTarget.targets" was not found. Confirm that the expression in the Import declaration ";..\13.1.1024\targets\Xamarin.Shared.Sdk.MultiTarget.targets" is correct, and that the file exists on disk.

What component do I need to install to get this?

AArnott · 2023-02-06T15:58:51Z

This works if I run init.ps1, restore, build works and even VS works if launched from that window. But VS launched from the start menu does not, nor does the command prompt until init.ps1 is run in that particular window. It seems that dotnet workload restore only works for a user-local unzipped copy of the .NET SDK. How can we get this to work in VS without having to spawn VS from an environment in which init.ps1 has just run?

sandyarmstrong · 2023-02-06T16:23:15Z

How can we get this to work in VS without having to spawn VS from an environment in which init.ps1 has just run?

I don't know. On Mac, we have a property we can add to a solution to tell us where it should look for the .NET SDK. I'm not aware of any way to do this on Windows.

Should we add a solution filter that removes the Mac projects from the solution that would be used by default?

AArnott · 2023-02-06T16:34:24Z

It isn't just the mac project. It causes all the projects to fail. The error I quoted appears for both Xunit.StaFact and Xunit.StaFact.Tests.Mac. And the presence of that error apparently causes nuget restore to altogether fail, so 302 compile errors are reported in the Error List across all projects.

I've finished reviewing the PR and pushed a few changes so that I'm happy with the result, except for the fact that the solution is unusable in VS except when launched from a command prompt after running init. That's a very big loss from this PR, so I'm holding out hope that we can resolve that before merging this.

AArnott · 2023-02-06T16:36:39Z

init.ps1

+    # Install specific macOS workload version in order to support usage on older .NET and macOS
+    $rollbackFile = Join-Path ([System.IO.Path]::GetTempPath()) ([System.Guid]::NewGuid())
+    @{
+        "microsoft.net.sdk.macos" = "12.3.2372/7.0.100"


Can you explain to me more about this requirement? Does this mean that dotnet workload restore is not enough?
I wonder too if this is somehow related to msbuild's failing import error.

The macOS workload is necessary in order to target net6.0-macos. That target framework is not available without the workload being installed.

The .NET SDK makes it difficult to specify a preferred version of a given workload. You have to use a rollback file to achieve this. Without specifying the version, the SDK will pull the latest for the SDK version specified in global.json (without taking target framework into account). So, even though this project is net6.0-macos, it would be referencing a newer Microsoft.macOS.dll that is not compatible with projects using the .NET 6 SDK and an older macos workload. To fix this, we specify an older workload version that is compatible with an older .NET/workload.

I wonder too if this is somehow related to msbuild's failing import error.

Yes. dotnet workload restore appears to be broken, at least with the macOS workload. It's like it tries to load targets before they have been installed. Tomas filed a bug about it recently: dotnet/sdk#30230. It would be great if the .NET SDK team fixed that. We can't depend on dotnet restore for CI, because it would pick up the wrong workload version as described earlier, but for local development it would be very convenient.

Thanks. That's helpful, and I've upvoted that issue.
How do dotnet workloads work when the SDK to be installed into is in an elevated location like Program Files?
It's very odd to me that init.ps1 works, but running this command outside of init fails, where workloads.json refers to the json file your init.ps1 creates:

> dotnet workload install macos --from-rollback-file workloads.json Downloading Microsoft.macOS.Sdk.Msi.x64 (13.1.1024) Workload installation failed. Rolling back installed packs... Workload installation failed: One or more errors occurred. (microsoft.macos.sdk.msi.x64::13.1.1024 is not found in NuGet feeds https://api.nuget.org/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/test-tools/nuget/v3/index.json".)

Notice it's picking 13.1.1024, which is a different version. It's newer, and I don't see it in nuget.

If you have multiple SDK versions installed, and you're running workload install from outside of a context where a global.json is specifying your SDK version, you can get different results based on whatever the default SDK is.

How do dotnet workloads work when the SDK to be installed into is in an elevated location like Program Files?

On Mac, when installing workloads to the machine-wide .NET SDK installation (/usr/local/share/dotnet), you have to use sudo to elevate. Presumably something similar is needed on Windows?

I do have the 7.0.200 installed, as of this morning when a VS update installed it. But all my dotnet workload install attempts were within the directory containing global.json, so evidently my repo's 7.0.101 constraint wasn't effective at retaining behavior from that SDK in this regard.

Windows doesn't need sudo to elevate. It prompts with interactive UI as needed.

The failures I'm seeing turn out to come from a collection of bugs in the SDK around this area. The do not show up when DOTNET_ROOT is set, which is why it works in non-elevated scenarios.

sandyarmstrong · 2023-02-06T17:06:52Z

It isn't just the mac project. It causes all the projects to fail. The error I quoted appears for both Xunit.StaFact and Xunit.StaFact.Tests.Mac. And the presence of that error apparently causes nuget restore to altogether fail, so 302 compile errors are reported in the Error List across all projects.

I've finished reviewing the PR and pushed a few changes so that I'm happy with the result, except for the fact that the solution is unusable in VS except when launched from a command prompt after running init. That's a very big loss from this PR, so I'm holding out hope that we can resolve that before merging this.

Yeah, that's really unfortunate. If we're lucky, the .NET SDK team will fix workload restore for .NET 8, but I wouldn't count on a fix being backported.

Perhaps we should disable the net6.0-macos target framework unless building on Mac or in CI? And in the rare cases where you want to build locally on Windows, you could remove that condition and just make sure to launch VS correctly?

SiyuGithub added 9 commits January 6, 2023 15:42

First verison

b026c64

Modify CocoaSynchronizationContextAdapter.cs pumptill method.

d7718fa

Modify PumpTill.

9516e6d

Modify more CocoaSynchronizationContext.cs and CocoaSynchronizationCo…

8fbeb9a

…ntextAdapter.cs

Modify version2

e996a5c

version3

897247b

Version4:

53bf326

Version5

fca1e86

Version6

0d636e2

SiyuGithub marked this pull request as draft January 11, 2023 18:57

AArnott reviewed Jan 13, 2023

View reviewed changes

SiyuGithub and others added 13 commits January 17, 2023 09:02

CocoaFact code clean up

68e2e49

More cleanup

04de106

Use Mac app as harness for xunit test runner

0bdc567

Need a full app bundle in order to start Cocoa mainloop.

Delete .DS_Store files

2ca5096

Ensure macos workload is installed

b621858

Convert to file-scoped namespaces

0695648

Cleanup

77e6854

Address a few extraneous warnings

a7cc92c

Only build Xunit.StaFact.Tests.Mac in *Mac configurations

5727671

Pass configuration when restoring on Mac

3cf676c

Basic workload-from-rollback provisioning

b13aa1d

This allows building on macOS 12 and easily consuming the nupkg from projects still using older workload versions.

Move Xunit.StaFact.Tests.Mac to .NET 6

153f2ec

Do not build for Mac on Linux

cf8b3a6

Code clean up

c6e76e7

sandyarmstrong reviewed Jan 30, 2023

View reviewed changes

SiyuGithub added 2 commits January 30, 2023 11:05

More code testing

d69935f

More clean up

1321b3c

SiyuGithub marked this pull request as ready for review January 31, 2023 21:20

SiyuGithub changed the title ~~Dev/siyu/start to add cocoa fact~~ Add cocoa fact attribute and related unit tests Feb 1, 2023

sandyarmstrong reviewed Feb 1, 2023

View reviewed changes

Test pipeline V1

b62abc3

AArnott added 4 commits February 6, 2023 08:14

Simplify the mac test project

892ca8b

Revert some sln file changes

bc5256d

revert spacing change

8f61172

Cleanup pipeline

4400fed

AArnott added 2 commits February 6, 2023 09:11

fix pipeline

795fe77

Avoid double-build on mac

f9e66fc

AArnott reviewed Feb 6, 2023

View reviewed changes

AArnott added 2 commits February 6, 2023 10:39

Use checked in workloads.json file

6a47cdd

Add notes to CONTRIBUTING about requirement to run

c04316a

AArnott enabled auto-merge (squash) February 6, 2023 18:09

AArnott merged commit f3d5569 into AArnott:main Feb 6, 2023

sandyarmstrong mentioned this pull request Feb 18, 2023

EditorFeatures.Cocoa should target net6.0-macos dotnet/roslyn#60078

Closed

AArnott mentioned this pull request Jun 5, 2023

[MainThreadFact] to execute tests on main thread on macos #58

Closed

AArnott added this to the v1.2 milestone Jun 5, 2023

dependabot bot mentioned this pull request Jul 26, 2025

Bump Xunit.StaFact to 3.0.13 lg2de/SimpleAccounting#354

Open

		Xunit.Sdk.CocoaFactDiscoverer
		Xunit.Sdk.CocoaFactDiscoverer.CocoaFactDiscoverer(Xunit.Abstractions.IMessageSink! diagnosticMessageSink) -> void

Add cocoa fact attribute and related unit tests #68

Add cocoa fact attribute and related unit tests #68

Uh oh!

Conversation

SiyuGithub commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandyarmstrong commented Jan 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott commented Jan 13, 2023

Uh oh!

AArnott Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandyarmstrong Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SiyuGithub commented Jan 31, 2023

Uh oh!

SiyuGithub commented Feb 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott commented Feb 6, 2023

Uh oh!

AArnott commented Feb 6, 2023

Uh oh!

sandyarmstrong commented Feb 6, 2023

Uh oh!

AArnott commented Feb 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandyarmstrong commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SiyuGithub commented Jan 11, 2023 •

edited

Loading

AArnott Jan 13, 2023 •

edited

Loading

sandyarmstrong Jan 13, 2023 •

edited

Loading

AArnott Feb 7, 2023 •

edited

Loading

sandyarmstrong commented Feb 6, 2023 •

edited

Loading