-
Notifications
You must be signed in to change notification settings - Fork 479
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.Qdrant #5099
Adding public API test coverage for Aspire.Hosting.Qdrant #5099
Conversation
…methods#aspire-hosting-qdrant
…methods#aspire-hosting-qdrant
…methods#aspire-hosting-qdrant
} | ||
|
||
[Fact] | ||
public void WithDataBindMountShouldThrowsWhenNameIsNull() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing the source
(source directory) parameter, and not name
.
var distributedApplicationBuilder = DistributedApplication.CreateBuilder([]); | ||
const string name = "Qdrant"; | ||
var apiKeyParameter = ParameterResourceBuilderExtensions.CreateDefaultPasswordParameter(distributedApplicationBuilder, $"{name}-Key", special: false); | ||
var resource = new QdrantServerResource(name, apiKeyParameter); | ||
var builder = distributedApplicationBuilder.AddResource(resource); | ||
string source = null!; | ||
|
||
var action = () => builder.WithDataBindMount(source); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need all this or would something like this be enough?:
IResourceBuilder<QdrantServerResource> builder = null!;
string source = null!;
var action = () => builder.WithDataBindMount(source);
@eerhardt @sebastienros thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it doesn't need to be that complicated. Even the Multiple
usage in all these tests, since the second assertion depends on the first, we don't care about the second result if the first one failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple
Indeed, I will remove Multiple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need all this or would something like this be enough?:
IResourceBuilder<QdrantServerResource> builder = null!; string source = null!; var action = () => builder.WithDataBindMount(source);@eerhardt @sebastienros thoughts?
But in this case. We will not handle the error when builder 'is not null' and source 'is null'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full test
var builder = TestDistributedApplicationBuilder.Create();
var qdrant = builder.AddQdrant("qdrant");
string source = null!;
var action = () => qdrant.WithDataBindMount(source);
var exception = Assert.Throws<ArgumentNullException>(action);
Assert.Equal(nameof(source), exception.ParamName);
…methods#aspire-hosting-qdrant
…methods#aspire-hosting-qdrant
…methods#aspire-hosting-qdrant
…methods#aspire-hosting-qdrant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the contribution!
Initial issues #2649
Issues Check List: Validate arguments of public methods
Microsoft Reviewers: Open in CodeFlow