Adjust SkipOnCoreClr for Match_ExcessPrefix test

[am11]

We wanted this test to skip on: RuntimeConfiguration.Checked or RuntimeTestModes.JitMinOpts,
but SkipOnCoreClr performs RuntimeConfiguration.Checked and RuntimeTestModes.JitMinOpts.

Related: #10680
Fixes: #59541

cc @stephentoub

[ghost]

Tagging subscribers to this area: @eerhardt, @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

Issue Details

---

We wanted this test to skip on: RuntimeConfiguration.Checked or RuntimeTestModes.JitMinOpts,
but SkipOnCoreClr performs RuntimeConfiguration.Checked and RuntimeTestModes.JitMinOpts.

Related: #10680
Fixes: #59541

cc @stephentoub

Author:	am11
Assignees:	-
Labels:	area-System.Text.RegularExpressions
Milestone:	-

[stephentoub]

but SkipOnCoreClr performs RuntimeConfiguration.Checked and RuntimeTestModes.JitMinOpts

Thanks. Are there other uses of SkipOnCoreClr that specify multiple conditions? Given that ConditionalXx runs the test only if all conditions pass, as the inverse it would logically make sense if SkipXx didn't run the test if any of them were an issue. So unless we're depending on this in other places, I'd be inclined to fix the attribute instead. But we should also validate other attributes to make sure we maintain consistent behavior so that they remain predictable.
cc: @safern

[am11]

Are there other uses of SkipOnCoreClr

There are only two other places where multiple conditions are used:

runtime % git grep 'SkipOnCoreClr.*,.*,' :/
src/libraries/System.Drawing.Common/tests/mono/System.Drawing/BitmapTests.cs:    [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/37082", TestPlatforms.AnyUnix, RuntimeConfiguration.Checked)]
src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs:        [SkipOnCoreClr("Long running tests: https://github.com/dotnet/runtime/issues/11980", TestPlatforms.Linux, RuntimeConfiguration.Checked)]

# ruling out multi-line case
runtime % git grep SkipOnCoreClr :/ | grep -v ']'
# nothing

Looks like we want && effect in those two places.

[am11]

Browser_wasm_Windows failure is #59556.

[safern]

So unless we're depending on this in other places, I'd be inclined to fix the attribute instead.

Maybe a better way to do this is change the SkipOnCoreClrAttribute to behave as the ActiveIssueAttribute which allows multiple attributes and that behaves as an OR. So that way you could add one attribute for Checked configuration and another attribute for the stress mode you want to skip the test.

https://github.com/dotnet/arcade/blob/e43b85e9b4e9ca62b89e407acdf0e56a2cbfd81a/src/Microsoft.DotNet.XUnitExtensions/src/Attributes/SkipOnCoreClrAttribute.cs#L10

ActiveIssueAttribute also behaves as an and for multiple parameters. If you want an OR, you add multiple attributes with different parameters.

[am11]

Maybe a better way to do this is change the SkipOnCoreClrAttribute to behave as the ActiveIssueAttribute which allows multiple attributes

Sounds good. Question, do we want to make the change in arcade and wait for arcade update (+ fix the skip attribute) to fix #59541? Or should we take this change (initial simple version 8756c60) to unblock clean CI and then update arcade lazily?

[safern]

Or should we take this change (initial simple version 8756c60) to unblock clean CI and then update arcade lazily?

If this is blocking PRs and making CI red, we should merge the quickest fix and then cleanup once the attribute supports multiple instances of it.

[am11]

Thanks, I have reverted it back to earlier version. I'll follow up on arcade (unless someone else beat me to it 🙂).

[ViktorHofer]

What's the status of this PR? Is it ready to be merged? Asking as this test failure nukes our pass rate (it's 0% right now).

[ViktorHofer]

cc @krwq

[stephentoub]

I've disabled it until the right fix is available

runtime/src/libraries/System.Text.RegularExpressions/tests/Regex.Match.Tests.cs

Line 1093 in 5d71e32

[ActiveIssue("https://github.com/dotnet/runtime/issues/59541")]

.

[am11]

Ah, arcade was updated today, just noticed that it missed the fix commit. 😕
Nonetheless, I have adapted the code, after the next arcade update, will re-merged main to PR branch.

[am11]

Today's arcade update brought the updated SkipOnCoreClrAttribute. All green. 🌴

[safern]

Thanks, @am11!