[suggestion] NUnit1028 could be improved

[Bartleby2718]

Problem 1:

nunit.analyzers/src/nunit.analyzers/NonTestMethodAccessibilityLevel/NonTestMethodAccessibilityLevelAnalyzer.cs

Lines 63 to 75 in e52e3a1

	private static bool IsPublicOrInternalMethod(IMethodSymbol method)
	{
	switch (method.DeclaredAccessibility)
	{
	case Accessibility.Public:
	case Accessibility.Internal:
	case Accessibility.ProtectedOrInternal:
	return true;
	default:
	return false;
	}
	}

clearly says that NUnit1028 detects public or internal methods, but the title, message, and description don't mention internal:

nunit.analyzers/src/nunit.analyzers/NonTestMethodAccessibilityLevel/NonTestMethodAccessibilityLevelConstants.cs

Lines 5 to 7 in e52e3a1

	internal const string NonTestMethodIsPublicTitle = "The non-test method is public";
	internal const string NonTestMethodIsPublicMessage = "Only test methods should be public";
	internal const string NonTestMethodIsPublicDescription = "A fixture should not contain any public non-test methods.";

Problem 2:

nunit.analyzers/src/nunit.analyzers/NonTestMethodAccessibilityLevel/NonTestMethodAccessibilityLevelCodeFix.cs

Lines 61 to 101 in e52e3a1

	private static SyntaxTokenList ReplaceModifiersWithPrivate(MethodDeclarationSyntax originalExpression)
	{
	var firstAccessModifier = true;
	var isProtected = false;
	var newSyntaxTokens = new List<SyntaxToken>();
	foreach (var syntaxToken in originalExpression.Modifiers)
	{
	if (IsPublicAccessModifier(syntaxToken))
	{
	if (firstAccessModifier && !isProtected)
	{
	firstAccessModifier = false;
	var newPrivateToken = SyntaxFactory.Token(
	syntaxToken.LeadingTrivia,
	SyntaxKind.PrivateKeyword,
	syntaxToken.TrailingTrivia);
	newSyntaxTokens.Add(newPrivateToken);
	}
	}
	else
	{
	if (syntaxToken.IsKind(SyntaxKind.ProtectedKeyword))
	isProtected = true;
	newSyntaxTokens.Add(syntaxToken);
	}
	}
	return new SyntaxTokenList(newSyntaxTokens);
	}
	private static bool HasExplicitPublicAccessModifier(MethodDeclarationSyntax methodDeclarationSyntax)
	{
	return methodDeclarationSyntax.Modifiers.Any(m => IsPublicAccessModifier(m));
	}
	private static bool IsPublicAccessModifier(SyntaxToken syntaxToken) =>
	syntaxToken.IsKind(SyntaxKind.PublicKeyword) ||
	syntaxToken.IsKind(SyntaxKind.InternalKeyword);
	}

makes the offending method private, but it should suggest so only if it isn't used anywhere else. Not sure if this can be done, but it should definitely be possible to reduce false positives.

[manfred-brands]

@Bartleby2718 Yes, the wording could be improved, to something like The non-test method is not private.
Unfortunately there is no way to see if the method is used elsewhere. The analyzer pass only knows about the current "file", not about others. The code fix is to help, not blindly follow. If method are really used it would involve moving the method to another helper class or so.

[marcusbrunello]

I am confused about this. Is it a good choice to make them internal?? NUnit1028 is complaining about them if internal ALSO.

[manfred-brands]

No. A test should be self-contained not expose methods to be called from elsewhere. Depending on what those methods do they could interfere with a test in progress and affect the result.
The only way to be certain is to only have private non-test methods.

If a method is shared between tests then they should be moved to a separate non-test class.

[marcusbrunello]

I see... and then how should those non-test classes be annotated?
Can you check/do you know if I have to annotate the last method in this group?
That test method is popping the NUnit1028 msg :( maybe because of the class being partial?

[SetUpFixture]
public partial class Testing
{
    private static ITestDatabase s_database;
    private static CustomWebApplicationFactory s_factory = null!;
    private static IServiceScopeFactory s_scopeFactory = null!;
    private static string? s_userId;

    [OneTimeSetUp]
    public async Task RunBeforeAnyTests()
    {
        s_database = await TestDatabaseFactory.CreateAsync();

        s_factory = new CustomWebApplicationFactory(s_database.GetConnection());

        s_scopeFactory = s_factory.Services.GetRequiredService<IServiceScopeFactory>();
    }

    internal static async Task<TResponse> SendAsync<TResponse>(IRequest<TResponse> request)
    {
        using var scope = s_scopeFactory.CreateScope();

        var mediator = scope.ServiceProvider.GetRequiredService<ISender>();

        return await mediator.Send(request);
    }

[manfred-brands]

For your case, I would removal all attributes from your class and create a new SetUpFixture class which calls Testing.RunBeforeAnyTests() in its OneTimeSetUp method.

Your test code can then keep calling Testing.SendAsync as before.