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

Adding public API test coverage for Aspire.Hosting.Valkey #5054

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bell-nat
Copy link

@bell-nat bell-nat commented Jul 24, 2024

Initial issues #2649
Issues Check List: Validate arguments of public methods

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-codeflow for labeling automated codeflow. intentionally a different color! label Jul 24, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2024
@bell-nat
Copy link
Author

@dotnet-policy-service agree

@radical radical added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication and removed area-codeflow for labeling automated codeflow. intentionally a different color! labels Jul 24, 2024
#region ValkeyBuilderExtensions

[Fact]
public void AddValkeyContainerShouldThrowsWhenBuilderIsNull()
Copy link
Member

Choose a reason for hiding this comment

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

nit: ShouldThrows -> ShouldThrow


var action = () => builder.AddValkey(name);

Assert.Multiple(() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on this comment. Assert.Multyple is not necessary.

You should just leave it here:
var exception = Assert.Throws(action);
Assert.Equal(nameof(builder), exception.ParamName);

Same for the code below

@bell-nat bell-nat changed the title CA1062#Aspire.Hosting.Valkey Adding public API test coverage for Aspire.Hosting.Valkey Jul 31, 2024
[Fact]
public void AddValkeyContainerShouldThrowWhenNameIsNull()
{
IDistributedApplicationBuilder builder = new DistributedApplicationBuilder([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use TestDistributedApplicationBuilder.Create();

{
string name = null!;

var action = () => new ValkeyResource(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a static check in the class
private static string ThrowIfNull([NotNull] string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null)
=> argument ?? throw new ArgumentNullException(paramName);


public class ValkeyPublicApiTests
{
#region ValkeyBuilderExtensions
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove Addrees pls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants