Skip to content

Refactor tests that cause warnings in CI#51730

Merged
RikkiGibson merged 2 commits intodotnet:mainfrom
Youssef1313:patch-32
Mar 7, 2021
Merged

Refactor tests that cause warnings in CI#51730
RikkiGibson merged 2 commits intodotnet:mainfrom
Youssef1313:patch-32

Conversation

@Youssef1313
Copy link
Member

Fixes #50337

@Youssef1313 Youssef1313 requested a review from a team as a code owner March 7, 2021 17:10
@ghost ghost added the Area-Compilers label Mar 7, 2021
}

public static IEnumerable<object[]> AnalyzeMethodsInEnabledContextOnly_01_Data()
// AnalyzeMethodsInEnabledContextOnly_01_Data is splitted due to https://github.com/dotnet/roslyn/issues/50337
Copy link
Contributor

Choose a reason for hiding this comment

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

"split"?

{
public class NullableContextTests : CSharpTestBase
{
private static readonly NullableDirectives[] s_nullableDirectives = new[]
Copy link
Contributor

Choose a reason for hiding this comment

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

s_nullableDirectives [](start = 53, length = 20)

Consider moving this immediately before AnalyzeMethodsInEnabledContextOnly_01_Data1().

[Theory]
[MemberData(nameof(AnalyzeMethodsInEnabledContextOnly_01_Data))]
[MemberData(nameof(AnalyzeMethodsInEnabledContextOnly_01_Data1))]
[MemberData(nameof(AnalyzeMethodsInEnabledContextOnly_01_Data2))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this address the issue? Don't we still have the same number of test results per test?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cston I'm waiting for CI to check. But I think while it's the same number per test, it's not the same number per test case. The warning is about "subresults in test case". But CI will say its decision 😄

Copy link
Contributor

@cston cston Mar 7, 2021

Choose a reason for hiding this comment

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

If the same issue exists, consider having two tests that share a common helper in each case.

[MemberData(nameof(AnalyzeMethodsInEnabledContextOnly_01_Data1)]
public void AnalyzeMethodsInEnabledContextOnly_01A(NullableContextOptions? projectContext,
    NullableDirectives classDirectives, NullableDirectives methodDirectives)
{
    AnalyzeMethodsInEnabledContextOnly_01_Execute(projectContext, classDirectives, methodDirectives);
}

[MemberData(nameof(AnalyzeMethodsInEnabledContextOnly_01_Data2)]
public void AnalyzeMethodsInEnabledContextOnly_01B(NullableContextOptions? projectContext,
    NullableDirectives classDirectives, NullableDirectives methodDirectives)
{
    AnalyzeMethodsInEnabledContextOnly_01_Execute(projectContext, classDirectives, methodDirectives);
}

private void AnalyzeMethodsInEnabledContextOnly_01_Execute(NullableContextOptions? projectContext,
    NullableDirectives classDirectives, NullableDirectives methodDirectives)
{
    ...
}

In reply to: 589068480 [](ancestors = 589068480)

Copy link
Member Author

Choose a reason for hiding this comment

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

@cston You're correct, the warnings are still there 😕

image

I don't have a good solution in mind. Duplicating the exact same test doesn't seem to be a good option. I'll close and let someone else come with a good solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cston Oh, I had the PR tab open and it didn't show your previous reply when I responded. I'll reopen and apply your change. Thanks.

@Youssef1313 Youssef1313 closed this Mar 7, 2021
@Youssef1313 Youssef1313 deleted the patch-32 branch March 7, 2021 18:34
@Youssef1313 Youssef1313 restored the patch-32 branch March 7, 2021 18:43
@Youssef1313 Youssef1313 reopened this Mar 7, 2021
@Youssef1313
Copy link
Member Author

Warnings are fixed 🎉

image

Thanks @cston

@RikkiGibson RikkiGibson added the Test Test failures in roslyn-CI label Mar 7, 2021
@RikkiGibson
Copy link
Member

Thanks @Youssef1313!

@RikkiGibson RikkiGibson merged commit b3e8749 into dotnet:main Mar 7, 2021
@Youssef1313 Youssef1313 deleted the patch-32 branch March 7, 2021 20:13
@allisonchou allisonchou added this to the Next milestone Mar 12, 2021
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Test Test failures in roslyn-CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

roslyn-ci has test publishing warnings due to too many subresults in test case

4 participants