Skip to content

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

@ghost ghost 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.

@danmoseley
Copy link
Member

@bell-nat are you expecting to be able to finish this PR? if not, I will close it with thanks for the start.

@danmoseley danmoseley added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 2, 2025
Copy link

This submission has been automatically marked as stale because it has been marked as requiring author action but has not had any activity for 14 days.
It will be closed if no further activity occurs within 7 days of this comment.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants