Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mono][xunit tests] Move skipped tests out of rsp file #2087

Merged

Conversation

MaximLipnin
Copy link
Contributor

Addresses #1980

@stephentoub
Copy link
Member

Thanks. What's the rhyme or reason for which lines were moved to be ActiveIssue and which ones weren't?

@MaximLipnin
Copy link
Contributor Author

Thanks. What's the rhyme or reason for which lines were moved to be ActiveIssue and which ones weren't?

I sent it as a draft with just several first tests moved in order to verify the general way. So I'm going to complete this work, at least for CoreFX.issues.rsp file, in the current PR.

@MaximLipnin
Copy link
Contributor Author

Thank you for a quick look!

@MaximLipnin
Copy link
Contributor Author

Given that the CoreFX.issues.rsp file is quite big, it would make sense to divide this work into several PRs and merge the current one because it's in good shape right now.

…ontainerTests.GetExportOfTTMetadataView1_TypeAsMetadataViewTypeArgument_IsUsedAsMetadataConstraint
…stentInDefaultContext and System.Runtime.Loader.Tests.DefaultLoadContextTests.LoadInDefaultContext from rsp file because the related issues should be addressed
…Tests.LoadNonExistentInDefaultContext and System.Runtime.Loader.Tests.DefaultLoadContextTests.LoadInDefaultContext because they fail on CI
@MaximLipnin MaximLipnin force-pushed the move_skipped_tests_out_of_rsp_file branch from 08cf306 to 315a4c8 Compare February 18, 2020 06:09
@MaximLipnin MaximLipnin marked this pull request as ready for review February 18, 2020 17:45
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

Looks great! I found a few small nits that can be addressed in a follow-up PR.


using Xunit;

[assembly: SkipOnMono("Flaky tests: https://github.com/mono/mono/issues/16417")]
Copy link
Member

Choose a reason for hiding this comment

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

should be

Suggested change
[assembly: SkipOnMono("Flaky tests: https://github.com/mono/mono/issues/16417")]
[assembly: ActiveIssue("Flaky tests: https://github.com/mono/mono/issues/16417", TestRuntimes.Mono)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Attribute 'ActiveIssue' is not valid on this declaration type. It is only valid on 'class, method' declarations.

Copy link
Member

Choose a reason for hiding this comment

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

Hm interesting. I don't see a good reason not to allow ActiveIssue on assembly, will file a PR in dotnet/arcade.

@@ -8,9 +8,11 @@

namespace System.ComponentModel.Composition
{
// [ActiveIssue("https://github.com/mono/mono/issues/16417", TestRuntimes.Mono)]
Copy link
Member

Choose a reason for hiding this comment

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

commented code, can be removed

Suggested change
// [ActiveIssue("https://github.com/mono/mono/issues/16417", TestRuntimes.Mono)]

#else
using nint = System.Int32;
using nuint = System.UInt32;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

nint/nuint isn't used, can be removed.

@akoeplinger akoeplinger merged commit b74422f into dotnet:master Feb 18, 2020
@MaximLipnin MaximLipnin deleted the move_skipped_tests_out_of_rsp_file branch February 19, 2020 05:22
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 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.

5 participants