refactor: Implemented analyzer suggestions and smaller refactorings (#…

…937)

* refactor: Implemented analyzer suggestions and smaller refactorings

* Use SuppressMessage instead of pragma

* ToList to ToArray

* fix: dont convert to array unnecessarily

* fix of the fix

Co-authored-by: Egil Hansen <egil@assimilated.dk>

src/bunit.core/Extensions/WaitForHelpers/RenderedFragmentWaitForHelperExtensions.cs

@@ -28,14 +28,9 @@ public static void WaitForState(this IRenderedFragmentBase renderedFragment, Fun
 		{
 			waiter.WaitTask.GetAwaiter().GetResult();
 		}
-		catch (Exception e)
+		catch (AggregateException e) when (e.InnerExceptions.Count == 1)
 		{
-			if (e is AggregateException aggregateException && aggregateException.InnerExceptions.Count == 1)
-			{
-				ExceptionDispatchInfo.Capture(aggregateException.InnerExceptions[0]).Throw();
-			}
-
-			throw;
+			ExceptionDispatchInfo.Capture(e.InnerExceptions[0]).Throw();
 		}
 	}
 
@@ -76,14 +71,9 @@ public static void WaitForAssertion(this IRenderedFragmentBase renderedFragment,
 		{
 			waiter.WaitTask.GetAwaiter().GetResult();
 		}
-		catch (Exception e)
+		catch (AggregateException e) when (e.InnerExceptions.Count == 1)
 		{
-			if (e is AggregateException aggregateException && aggregateException.InnerExceptions.Count == 1)
-			{
-				ExceptionDispatchInfo.Capture(aggregateException.InnerExceptions[0]).Throw();
-			}
-
-			throw;
+			ExceptionDispatchInfo.Capture(e.InnerExceptions[0]).Throw();
 		}
 	}
 

src/bunit.core/Rendering/RootRenderTree.cs

@@ -7,6 +7,7 @@ namespace Bunit.Rendering;
 /// Components added to the render tree must have either a <c>ChildContent</c> or
 /// <c>Body</c> parameter.
 /// </summary>
+[SuppressMessage("Naming", "CA1710:Identifiers should have correct suffix", Justification = "A tree is a collection by default.")]
 public sealed class RootRenderTree : IReadOnlyCollection<RootRenderTreeRegistration>
 {
 	private readonly List<RootRenderTreeRegistration> registrations = new();

src/bunit.core/StringSyntaxAttribute.cs

@@ -2,7 +2,8 @@
 namespace System.Diagnostics.CodeAnalysis;
 
 /// <summary>Fake version of the StringSyntaxAttribute, which was introduced in .NET 7</summary>
-[AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Field | AttributeTargets.Property, AllowMultiple = false, Inherited = false)]
+[AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Field | AttributeTargets.Property)]
+[SuppressMessage("Design", "CA1019:Define accessors for attribute arguments", Justification = "The sole purpose is to have the same public surface as the class in .NET7 and above.")]
 public sealed class StringSyntaxAttribute : Attribute
 {
 	/// <summary>

src/bunit.web/Asserting/DiffAssertExtensions.cs

@@ -18,10 +18,18 @@ public static IDiff ShouldHaveSingleChange(this IEnumerable<IDiff> diffs)
 	{
 		if (diffs is null)
 			throw new ArgumentNullException(nameof(diffs));
-		if (diffs.Take(2).Count() != 1) // Optimized way of writing "diffs.Count() != 1"
-			throw new ActualExpectedAssertException(diffs.Count().ToString(CultureInfo.InvariantCulture), "1", "Actual changes", "Expected changes", "There were more than one change");
 
-		return diffs.First();
+		var diffsArray = diffs.ToArray();
+
+		if (diffsArray.Length != 1)
+			throw new ActualExpectedAssertException(
+				actual: diffsArray.Length.ToString(CultureInfo.InvariantCulture),
+				expected: "1",
+				actualText: "Actual changes",
+				expectedText: "Expected changes",
+				message: "There were more than one change");
+
+		return diffsArray.First();
 	}
 
 	/// <summary>
@@ -38,11 +46,18 @@ public static void ShouldHaveChanges(this IEnumerable<IDiff> diffs, params Actio
 		if (diffInspectors is null)
 			throw new ArgumentNullException(nameof(diffInspectors));
 
-		if (diffs.Take(diffInspectors.Length + 1).Count() != diffInspectors.Length) // Optimized way of writing "diffs.Count() != diffInspectors.Length"
-			throw new ActualExpectedAssertException(diffs.Count().ToString(CultureInfo.InvariantCulture), diffInspectors.Length.ToString(CultureInfo.InvariantCulture), "Actual changes", "Expected changes", "The actual number of changes does not match the expected.");
+		var diffsArray = diffs.ToArray();
+
+		if (diffsArray.Length != diffInspectors.Length)
+			throw new ActualExpectedAssertException(
+				actual: diffsArray.Length.ToString(CultureInfo.InvariantCulture),
+				expected: diffInspectors.Length.ToString(CultureInfo.InvariantCulture),
+				actualText: "Actual changes",
+				expectedText: "Expected changes",
+				message: "The actual number of changes does not match the expected.");
 
-		int index = 0;
-		foreach (var diff in diffs)
+		var index = 0;
+		foreach (var diff in diffsArray)
 		{
 			diffInspectors[index](diff);
 			index++;

src/bunit.web/Extensions/WaitForHelpers/RenderedFragmentWaitForHelperExtensions.cs

@@ -159,13 +159,11 @@ private static IElement WaitForElementCore(this IRenderedFragment renderedFragme
 		{
 			return waiter.WaitTask.GetAwaiter().GetResult();
 		}
-		catch (Exception e)
+		catch (AggregateException e) when (e.InnerExceptions.Count == 1)
 		{
-			if (e is AggregateException aggregateException && aggregateException.InnerExceptions.Count == 1)
-			{
-				ExceptionDispatchInfo.Capture(aggregateException.InnerExceptions[0]).Throw();
-			}
+			ExceptionDispatchInfo.Capture(e.InnerExceptions[0]).Throw();
 
+			// Unreachable code
 			throw;
 		}
 	}
@@ -189,13 +187,11 @@ private static IRefreshableElementCollection<IElement> WaitForElementsCore(
 		{
 			return waiter.WaitTask.GetAwaiter().GetResult();
 		}
-		catch (Exception e)
+		catch (AggregateException e) when (e.InnerExceptions.Count == 1)
 		{
-			if (e is AggregateException aggregateException && aggregateException.InnerExceptions.Count == 1)
-			{
-				ExceptionDispatchInfo.Capture(aggregateException.InnerExceptions[0]).Throw();
-			}
+			ExceptionDispatchInfo.Capture(e.InnerExceptions[0]).Throw();
 
+			// Unreachable code
 			throw;
 		}
 	}

src/bunit.web/JSInterop/JSRuntimeInvocationDictionary.cs

@@ -15,12 +15,7 @@ public sealed class JSRuntimeInvocationDictionary : IReadOnlyCollection<JSRuntim
 	/// </summary>
 	/// <param name="identifier">The identifier to get invocations for.</param>
 	/// <returns>An <see cref="IReadOnlyList{JSRuntimeInvocation}"/>.</returns>
-	public IReadOnlyList<JSRuntimeInvocation> this[string identifier]
-	{
-		get => invocations.ContainsKey(identifier)
-			? invocations[identifier]
-			: Array.Empty<JSRuntimeInvocation>();
-	}
+	public IReadOnlyList<JSRuntimeInvocation> this[string identifier] => invocations.TryGetValue(identifier, out var value) ? value : Array.Empty<JSRuntimeInvocation>();
 
 	/// <summary>
 	/// Gets a read only collection of all the identifiers used in invocations in this dictionary.

src/bunit.web/TestDoubles/Authorization/FakeAuthorizationService.cs

@@ -66,17 +66,20 @@ public Task<AuthorizationResult> AuthorizeAsync(ClaimsPrincipal user, object? re
 
 		AuthorizationResult result;
 
-		if (requirements.All(p => p is DenyAnonymousAuthorizationRequirement))
+		var requirementsArray = requirements.ToArray();
+		if (requirementsArray.All(p => p is DenyAnonymousAuthorizationRequirement))
 		{
-			result = (currentState == AuthorizationState.Authorized) ? AuthorizationResult.Success() : AuthorizationResult.Failed();
+			result = currentState == AuthorizationState.Authorized
+				? AuthorizationResult.Success()
+				: AuthorizationResult.Failed();
 		}
-		else if (requirements.All(p => p is RolesAuthorizationRequirement))
+		else if (requirementsArray.All(p => p is RolesAuthorizationRequirement))
 		{
-			result = VerifyRequiredRoles(requirements);
+			result = VerifyRequiredRoles(requirementsArray);
 		}
 		else if (supportedPolicies is not null)
 		{
-			result = VerifyRequiredPolicies(requirements);
+			result = VerifyRequiredPolicies(requirementsArray);
 		}
 		else
 		{
@@ -103,35 +106,28 @@ public Task<AuthorizationResult> AuthorizeAsync(ClaimsPrincipal user, object? re
 		return AuthorizeAsync(user, resource, requirements);
 	}
 
-	private AuthorizationResult VerifyRequiredPolicies(IEnumerable<IAuthorizationRequirement> requirements)
+	private AuthorizationResult VerifyRequiredPolicies(IReadOnlyCollection<IAuthorizationRequirement> requirements)
 	{
 		if (supportedPolicies.IsNullOrEmpty() || requirements.IsNullOrEmpty())
 		{
 			return AuthorizationResult.Failed();
 		}
 
-		foreach (IAuthorizationRequirement req in requirements)
-		{
-			if (req is TestPolicyRequirement testReq && supportedPolicies.Contains(testReq.PolicyName, StringComparer.Ordinal))
-				return AuthorizationResult.Success();
-		}
-
-		return AuthorizationResult.Failed();
+		return requirements.OfType<TestPolicyRequirement>().Any(req => supportedPolicies.Contains(req.PolicyName, StringComparer.Ordinal))
+			? AuthorizationResult.Success()
+			: AuthorizationResult.Failed();
 	}
 
-	private AuthorizationResult VerifyRequiredRoles(IEnumerable<IAuthorizationRequirement> requirements)
+	private AuthorizationResult VerifyRequiredRoles(IReadOnlyCollection<IAuthorizationRequirement> requirements)
 	{
-		AuthorizationResult result = AuthorizationResult.Failed();
-		foreach (IAuthorizationRequirement req in requirements)
+		var result = AuthorizationResult.Failed();
+		foreach (var req in requirements.OfType<RolesAuthorizationRequirement>())
 		{
-			if (req is RolesAuthorizationRequirement testReq)
+			var rolesFound = req.AllowedRoles.Intersect(supportedRoles, StringComparer.Ordinal);
+			if (rolesFound.Any())
 			{
-				IEnumerable<string> rolesFound = testReq.AllowedRoles.Intersect(supportedRoles, StringComparer.Ordinal);
-				if (rolesFound.Any())
-				{
-					result = AuthorizationResult.Success();
-					break;
-				}
+				result = AuthorizationResult.Success();
+				break;
 			}
 		}
 

tests/bunit.core.tests/ComponentFactories/GenericComponentFactoryTest.cs

@@ -25,7 +25,7 @@ public void Test002()
 		cut.MarkupMatches(@"<div id=""ref-status"">Has ref = True</div>");
 	}
 
-	private class FakeSimple1 : Simple1
+	private sealed class FakeSimple1 : Simple1
 	{
 		protected override void OnInitialized() { }
 		protected override Task OnInitializedAsync() => Task.CompletedTask;

tests/bunit.core.tests/ComponentParameterCollectionBuilderTests.cs

@@ -720,9 +720,9 @@ private class Params : ComponentBase
 		public void SomeMethod() { }
 	}
 
-	private class NoParams : ComponentBase { }
-	private class NonChildContentParameter : ComponentBase { public RenderFragment? ChildContent { get; set; } }
-	private class InhertedParams : Params { }
+	private sealed class NoParams : ComponentBase { }
+	private sealed class NonChildContentParameter : ComponentBase { public RenderFragment? ChildContent { get; set; } }
+	private sealed class InhertedParams : Params { }
 	private abstract class ParamsBase<T> : ComponentBase
 	{
 		public abstract T Value { get; set; }
@@ -733,35 +733,35 @@ private class InheritedParamsWithOverride : ParamsBase<bool?>
 		[Parameter] public override bool? Value { get; set; }
 	}
 
-	private class InheritedParamsWithoutOverride : InheritedParamsWithOverride
+	private sealed class InheritedParamsWithoutOverride : InheritedParamsWithOverride
 	{ }
 
-	private class TemplatedChildContent : ComponentBase
+	private sealed class TemplatedChildContent : ComponentBase
 	{
 		[Parameter] public RenderFragment<string>? ChildContent { get; set; }
 	}
 
-	private class NoTwoWayBind : ComponentBase
+	private sealed class NoTwoWayBind : ComponentBase
 	{
 		[Parameter]
 		public string Value { get; set; }
 	}
 
-	private class InvalidTwoWayBind : ComponentBase
+	private sealed class InvalidTwoWayBind : ComponentBase
 	{
 		[Parameter]
 		public string Value { get; set; }
 
 		public EventCallback<string> ValueChanged { get; set; }
 	}
 
-	private class ComponentWithCascadingParameter : ComponentBase
+	private sealed class ComponentWithCascadingParameter : ComponentBase
 	{
 		[CascadingParameter] public string Value { get; set; } = string.Empty;
 		[Parameter] public EventCallback<string> ValueChanged { get; set; }
 	}
 
-	private class ValidNamesComponent : ComponentBase
+	private sealed class ValidNamesComponent : ComponentBase
 	{
 		[Parameter] public DateTime LastChanged { get; set; }
 		[Parameter] public EventCallback<DateTime> LastChangedChanged { get; set; }

tests/bunit.core.tests/ComponentParameterCollectionTest.cs

@@ -335,7 +335,7 @@ public void Test055()
 		Should.Throw<ArgumentException>(() => sut.ToRenderFragment<Params>());
 	}
 
-	private class Params : ComponentBase
+	private sealed class Params : ComponentBase
 	{
 		public const string TemplateContent = "FOO";
 

tests/bunit.core.tests/ComponentParameterFactoryTest.cs

@@ -263,7 +263,7 @@ public void Test063()
 		renderedFragment.Markup.ShouldBe(nameof(TestComponent) + EXPECTED);
 	}
 
-	private class TestComponent : ComponentBase
+	private sealed class TestComponent : ComponentBase
 	{
 		[Parameter] public string? Input { get; set; }
 		[Parameter] public RenderFragment<string>? Template { get; set; }

tests/bunit.core.tests/Extensions/WaitForHelpers/RenderedFragmentWaitForHelperExtensionsTest.cs

@@ -129,7 +129,7 @@ public void Test301()
 			() => cut.WaitForState(() => false, TimeSpan.FromSeconds(5)));
 	}
 
-	internal class ThrowsAfterAsyncOperation : ComponentBase
+	private sealed class ThrowsAfterAsyncOperation : ComponentBase
 	{
 		protected override async Task OnInitializedAsync()
 		{

tests/bunit.core.tests/Rendering/RootRenderTreeTest.cs

@@ -144,7 +144,7 @@ public void Test123()
 			x => x.ComponentType.ShouldBe(typeof(CascadingValue<int>)));
 	}
 
-	private class LayoutComponent : LayoutComponentBase
+	private sealed class LayoutComponent : LayoutComponentBase
 	{
 		[Parameter] public string Value { get; set; } = "LAYOUT VALUE";
 		[Parameter] public string? Name { get; set; }
@@ -160,7 +160,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder)
 		}
 	}
 
-	private class InnerComponent : ComponentBase
+	private sealed class InnerComponent : ComponentBase
 	{
 		[CascadingParameter] public string LayoutValue { get; set; } = string.Empty;
 
@@ -172,7 +172,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder)
 		}
 	}
 
-	private class MultipleParametersInnerComponent : ComponentBase
+	private sealed class MultipleParametersInnerComponent : ComponentBase
 	{
 		[CascadingParameter] public string StringValue { get; set; } = string.Empty;
 		[CascadingParameter] public int IntValue { get; set; }