Disable DynamicCode_Jit test from System.Runtime.Tests on Mono

[EgorBo]

Crashes on mono with interpreter (IsDynamicCodeCompiled is false). Does this test make any sense? There is already https://github.com/EgorBo/runtime/blob/93ad80eb64aabc2df2d93a136c2f86b73f590b87/src/libraries/System.Runtime/tests/System/Runtime/CompilerServices/RuntimeFeatureTests.cs#L20-L29

or I should wrap the test with some check (e.g. IsCoreCLR or !isMono etc)

[stephentoub]

Crashes on mono

Crashes? Or the test just fails?

If it crashes, that should be fixed in mono.

Once #118 is merged, the same attributes should be usable to disable this on mono (right, @safern?)

[stephentoub]

We shouldn't delete this. We can make it coreclr-specific if it not correct on mono, or better yet, add the corresponding test for mono's results.

[akoeplinger]

Depending on where and how Mono is used all of these combinations are valid, e.g. Desktop Mono returns true for both, but Xamarin.iOS returns false for both, except if the interpreter is enabled then IsDynamicCodeSupported is true and IsDynamicCodeCompiled is false.

I'd say we make this test CoreCLR specific for now.

[EgorBo]

Yeah we currently don't have a C# API to detect current mode (is fullaot? is interp? etc) properly so I disabled the test for mono.

[akoeplinger]

Crashes? Or the test just fails?

It fails.

[safern]

Once #118 is merged, the same attributes should be usable to disable this on mono (right, @safern?)

Right, not exactly the same attribute, we added one specific for mono SkipOnMonoAttribute. I plan to submit a follow up PR to skip mono tests.

[akoeplinger]

Does this PR need to wait until the new attribute is available or can we merge it as is?

[safern]

Does this PR need to wait until the new attribute is available or can we merge it as is?

The new attributes should now be available. We got an arcade update yesterday.

So it would be nice to use them for this PR instead. Here is the PR that added them and it has examples on how to use them: dotnet/arcade#4231

[MihaZupan]

@akoeplinger looks like this is failing tests on all other PRs now due to the missing reason on SkipOnMono

[safern]

I don't even understand how this build was green. That's really odd, but yeah reason is missing:

https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.XUnitExtensions/src/Attributes/SkipOnMonoAttribute.cs#L12

[safern]

I created: #324

[akoeplinger]

@safern so I checked the build results that are attached to 5f3f3c9 (the latest commit in this PR) and the build actually checked out e45b79d!

Look for the HEAD is now at 3fe34b79 Merge e45b79d26fcee9770842c9681e6907430b08aebc into bf88f146412d1e2d41779422337184a802f186c9 message in Checkout step.

This means it ran against a previous version of the code. Sounds like some weird AzDO bug, did you see this previously?

[EgorBo]

Oops. sorry, didn't notice the argument and wanted to test on CI and it somehow validated it.

[MihaZupan]

I've opened a ticket on this CI behavior

[safern]

I believe @jashook has mentioned this someday last week and that they let AzDo know already but I’ll let him confirm.