Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
akoeplinger merged 96 commits into from
Feb 18, 2020
Merged

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

akoeplinger merged 96 commits into from
Feb 18, 2020

Conversation

@MaximLipnin
Copy link
Contributor

@MaximLipnin MaximLipnin commented Jan 23, 2020

Addresses #1980

@stephentoub
Copy link
Member

stephentoub commented Jan 23, 2020

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

akoeplinger
akoeplinger requested changes Jan 23, 2020
View reviewed changes
@MaximLipnin
Copy link
Contributor Author

MaximLipnin commented Jan 24, 2020

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

MaximLipnin commented Jan 24, 2020

Thank you for a quick look!

@MaximLipnin MaximLipnin force-pushed the move_skipped_tests_out_of_rsp_file branch from 07ec772 to bc6dd61 Compare January 24, 2020 11:58
@MaximLipnin MaximLipnin mentioned this pull request Jan 24, 2020
Closed
@MaximLipnin
Copy link
Contributor Author

MaximLipnin commented Jan 24, 2020

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.

@MaximLipnin MaximLipnin force-pushed the move_skipped_tests_out_of_rsp_file branch from 37a2154 to 7a0b55a Compare January 27, 2020 08:23
This was referenced Jan 27, 2020
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Jan 28, 2020
Closed
Closed
Merged
@MaximLipnin MaximLipnin force-pushed the move_skipped_tests_out_of_rsp_file branch from 905511b to 092faed Compare January 29, 2020 08:23
MaximLipnin
MaximLipnin commented Jan 29, 2020
View reviewed changes
This was referenced Jan 30, 2020
Open
Open
@MaximLipnin MaximLipnin force-pushed the move_skipped_tests_out_of_rsp_file branch from 3523e4b to 25119d3 Compare January 31, 2020 09:17
This was referenced Jan 31, 2020
Closed
Closed
Closed
Closed
Closed
@MaximLipnin
Add an active issue to System.ComponentModel.Composition.MetadataAttr…
b7aa5d3 
…ibuteTests.StronglyTypedStructureTest
@MaximLipnin
Add an active issue to System.ComponentModel.Composition.CompositionC…
3b74374 
…ontainerTests.GetExportOfTTMetadataView1_TypeAsMetadataViewTypeArgument_IsUsedAsMetadataConstraint
@MaximLipnin
Disable the whole System.ComponentModel.Composition.Tests namespace
5514c90
@MaximLipnin
Move some System.Runtime.Loader.Tests, which rely on collectible Asse…
05fe86d 
…mblyLoadContext, out of rsp file
@MaximLipnin
Remove three System.Runtime.Loader.Tests.AssemblyLoadContextTest test…
07afeb2 
…s with no local failures from rsp file
@MaximLipnin
Remove System.Runtime.Loader.Tests.DefaultLoadContextTests.LoadNonExi…
6a0d104 
…stentInDefaultContext and System.Runtime.Loader.Tests.DefaultLoadContextTests.LoadInDefaultContext from rsp file because the related issues should be addressed
@MaximLipnin
Add an active issue to System.Runtime.Loader.Tests.DefaultLoadContext…
3ce15ea 
…Tests.LoadNonExistentInDefaultContext and System.Runtime.Loader.Tests.DefaultLoadContextTests.LoadInDefaultContext because they fail on CI
@MaximLipnin
Move several System.Runtime.Loader.Tests.AssemblyLoadContextTest with…
7779a6b 
… an active issues out of rsp file
@MaximLipnin
Remove System.Utf8String.Experimental.Tests from rsp file to verify h…
315a4c8 
…ow they behave 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
akoeplinger
akoeplinger approved these changes Feb 18, 2020
View reviewed changes
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.

src/libraries/System.ComponentModel.Composition/tests/AssemblyInfo.cs

using Xunit;

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

@akoeplinger akoeplinger Feb 18, 2020

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

@MaximLipnin MaximLipnin Feb 19, 2020

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

@akoeplinger akoeplinger Feb 19, 2020

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.

...on/tests/System/ComponentModel/Composition/AttributedModel/AllowNonPublicCompositionTests.cs

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

@akoeplinger akoeplinger Feb 18, 2020

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)]

src/mono/netcore/System.Private.CoreLib/src/System/Utf8String.cs
#else
using nint = System.Int32;
using nuint = System.UInt32;
#endif
Copy link
Member

@akoeplinger akoeplinger Feb 18, 2020

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
@MaximLipnin MaximLipnin mentioned this pull request Feb 19, 2020
Merged
@akoeplinger akoeplinger mentioned this pull request Mar 9, 2020
Closed
@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.

Reviewers

@akoeplinger akoeplinger akoeplinger approved these changes

@jkotas jkotas jkotas left review comments

@marek-safar marek-safar marek-safar approved these changes

@ahsonkhan ahsonkhan Awaiting requested review from ahsonkhan

@EgorBo EgorBo Awaiting requested review from EgorBo

@steveisok steveisok Awaiting requested review from steveisok

@vargaz vargaz Awaiting requested review from vargaz

Assignees

No one assigned

Labels

area-Infrastructure-libraries

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@MaximLipnin @stephentoub @marek-safar @akoeplinger @jkotas