-
Notifications
You must be signed in to change notification settings - Fork 739
Split redis tests into separate test assembly #4468
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
Changes from 3 commits
50a9e63
8d36ff8
c6a1655
fbda632
3488419
98d2f86
7e9eef0
c8be7b3
6462358
1394ee8
7268fe3
6818dab
03e54d3
153f97b
0859fb6
497e633
c55aa34
9bc54e9
69f4ea9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
|
|
||
| <ItemGroup> | ||
| <InternalsVisibleTo Include="Aspire.Hosting.Tests" /> | ||
| <InternalsVisibleTo Include="Aspire.Hosting.Redis.Tests" /> | ||
|
||
| </ItemGroup> | ||
|
|
||
| </Project> | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||||||
| <Project Sdk="Microsoft.NET.Sdk"> | ||||||||||
|
|
||||||||||
| <PropertyGroup> | ||||||||||
| <TargetFramework>$(NetCurrent)</TargetFramework> | ||||||||||
| <IsAspireHost>true</IsAspireHost> | ||||||||||
| <!-- | ||||||||||
| CS8002: Referenced assembly does not have a strong name. MongoDB.Driver package is unsigned, we ignore that warning on purpose | ||||||||||
| --> | ||||||||||
| <NoWarn>$(NoWarn);CS8002</NoWarn> | ||||||||||
| </PropertyGroup> | ||||||||||
|
|
||||||||||
| <ItemGroup> | ||||||||||
| <ProjectReference Include="..\..\src\Aspire.Hosting.AppHost\Aspire.Hosting.AppHost.csproj" IsAspireProjectResource="false" /> | ||||||||||
| <ProjectReference Include="..\..\src\Aspire.Hosting.Redis\Aspire.Hosting.Redis.csproj" IsAspireProjectResource="false" /> | ||||||||||
|
||||||||||
| <ProjectReference Include="..\..\src\Aspire.Hosting.AppHost\Aspire.Hosting.AppHost.csproj" IsAspireProjectResource="false" /> | |
| <ProjectReference Include="..\..\src\Aspire.Hosting.Redis\Aspire.Hosting.Redis.csproj" IsAspireProjectResource="false" /> | |
| <ProjectReference Include="..\..\src\Aspire.Hosting.AppHost\Aspire.Hosting.AppHost.csproj" /> | |
| <ProjectReference Include="..\..\src\Aspire.Hosting.Redis\Aspire.Hosting.Redis.csproj" /> |
Are these needed? This project doesn't have <IsAspireHost>true</IsAspireHost>
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.
Works as you suggest
sebastienros marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| <Project> | ||
|
|
||
| <Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" /> | ||
|
|
||
| <!-- NOTE: This line is only required because we are using P2P references, not NuGet. It will not exist in real apps. --> | ||
| <Import Project="../../src/Aspire.Hosting.AppHost/build/Aspire.Hosting.AppHost.props" /> | ||
sebastienros marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| </Project> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| <Project> | ||
|
|
||
| <Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.targets', '$(MSBuildThisFileDirectory)../'))" /> | ||
|
|
||
| <!-- NOTE: This line is only required because we are using P2P references, not NuGet. It will not exist in real apps. --> | ||
| <Import Project="..\..\src\Aspire.Hosting.AppHost\build\Aspire.Hosting.AppHost.targets" /> | ||
|
|
||
| </Project> |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,49 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||||||||||||||||||||||||||||||||||||||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| using Aspire.Hosting.Utils; | ||||||||||||||||||||||||||||||||||||||||||||||
| using Microsoft.Extensions.Configuration; | ||||||||||||||||||||||||||||||||||||||||||||||
| using Microsoft.Extensions.DependencyInjection; | ||||||||||||||||||||||||||||||||||||||||||||||
| using Microsoft.Extensions.Hosting; | ||||||||||||||||||||||||||||||||||||||||||||||
| using StackExchange.Redis; | ||||||||||||||||||||||||||||||||||||||||||||||
| using Xunit; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| namespace Aspire.Hosting.Redis.Tests; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| public class RedisFunctionalTests | ||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||
| [Fact] | ||||||||||||||||||||||||||||||||||||||||||||||
| public async Task VerifyRedisResource() | ||||||||||||||||||||||||||||||||||||||||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @eerhardt @radical @ReubenBond @mitchdenny thoughts?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this test adds value over top of the other Redis Hosting tests, since it actually connects a client to it. It seems like a "middle of the ground" test. Not a unit test, but not a full end-to-end functional test like aspire/tests/Aspire.EndToEnd.Tests/IntegrationServicesTests.cs Lines 32 to 53 in 8e311f3
It is definitely simpler than the end-to-end tests.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like these tests, they are a good smoke test. I do think they need to be broken up though. That way if I've made some changes in "redis" land I can run everything related to Redis just by pointing at a particular test assembly.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's testing less than the current E2E tests, at least when looking from the Redis POV. The E2E is only creating a distinct web api project to use the component, that's the difference with this local functional test. Do we still need the Redis portion of the E2E test if so? Or should we extract the current portion of the E2E test into a separate project? For instance, could we not add more volumes testing with this new functional test project already?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The part that the current E2E tests cover that aren't covered here is the "WithReference" portion that connects a separate project with a resource. This is being simulated in the current test with: hb.Configuration.AddInMemoryCollection(new Dictionary<string, string?>
{
["ConnectionStrings:redis"] = await redis.Resource.GetConnectionStringAsync()
});
I think once we have all the AppHost resource tests split into separate test assemblies, and FunctionalTests like this one for each resource, we can remove the bulk of the resources from the E2E test, and only keep maybe ~5 core resources being tested there. That way when a new resource/component gets added we don't need to update the E2E test and have it continually grow, like it is doing today. |
||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||
| var builder = TestDistributedApplicationBuilder.Create(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| var redis = builder.AddRedis("redis"); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| using var app = builder.Build(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| await app.StartAsync(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| var hb = Host.CreateApplicationBuilder(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| hb.Configuration.AddInMemoryCollection(new Dictionary<string, string?> | ||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||
| ["ConnectionStrings:redis"] = await redis.Resource.GetConnectionStringAsync() | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| hb.AddRedisClient("redis"); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| using var host = hb.Build(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| await host.StartAsync(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| var redisClient = host.Services.GetRequiredService<IConnectionMultiplexer>(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| var db = redisClient.GetDatabase(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| await db.StringSetAsync("key", "value"); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| var value = await db.StringGetAsync("key"); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| Assert.Equal("value", value); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
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.
We should aim to remove this.