Skip to content

Update SkipOnCoreClrAttributes to new parameters#28875

Closed
safern wants to merge 1 commit into
dotnet:masterfrom
safern:UpdateSkipOnCoreClr
Closed

Update SkipOnCoreClrAttributes to new parameters#28875
safern wants to merge 1 commit into
dotnet:masterfrom
safern:UpdateSkipOnCoreClr

Conversation

@safern
Copy link
Copy Markdown
Member

@safern safern commented Feb 1, 2020

Depends on: dotnet/arcade#4724 change to flow via arcade.

@safern safern requested a review from stephentoub February 1, 2020 01:11
namespace System.ComponentModel.Composition.Registration.Tests
{
[SkipOnCoreClr("Test failures on stress tests")]
[SkipOnCoreClr("Test failures on stress tests", RuntimeTestModes.CheckedRuntime)]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it just checked builds, or any of the stress / non-release configurations?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would apply to ALL Checked builds, no matter if it is stress or not.

However, I just realized that the current design is not great as if we added a | RuntimeTestModes.GCStress it would mean, ALL Checked or GCStress (Checked or Release), as I would think what we want is that if I say RuntimeTestModes.Checked it would mean, ALL Checked, but if I add an stress mode flag, then it would mean, ONLY skip when running GCStress when in Checked. What is your expectation/thought for that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub thoughts on that?

I'll include this change in the arcade update to unblock and can update again based on your thoughts. I think updating it to the latter is the ideal.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I gave it a thought and put up a PR to update it to a better way of doing it: dotnet/arcade#4764


[assembly: SkipOnCoreClr("System.Net.Tests are flaky and/or long running: https://github.com/dotnet/runtime/issues/131")]
[assembly: SkipOnCoreClr("System.Net.Tests are flaky and/or long running: https://github.com/dotnet/runtime/issues/131", RuntimeTestModes.CheckedRuntime)]
[assembly: SkipOnMono("System.Net.Tests are flaky and/or long running: https://github.com/dotnet/runtime/issues/131")]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know if mono actually had problems with these or just copied the coreclr attribution thinking it was about all configs and not just stress?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption here is that mono did have problems with this, and ActiveIssue isn't usable on an assembly level. @akoeplinger ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I just copied from CoreCLR so this might not be needed.

@safern
Copy link
Copy Markdown
Member Author

safern commented Feb 5, 2020

Closing as I included this change in: #19613

@safern safern closed this Feb 5, 2020
@safern safern deleted the UpdateSkipOnCoreClr branch February 5, 2020 04:01
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants