Split redis tests into separate test assembly #4468

davidfowl · 2024-06-11T21:44:46Z

Initial stab at #4294.

I added solution folders for tests:

TODO:

I need to make to move the functional tests for redis.

Microsoft Reviewers: Open in CodeFlow

src/Aspire.Hosting/Aspire.Hosting.csproj

davidfowl · 2024-06-11T21:45:14Z

src/Aspire.Hosting.Redis/Aspire.Hosting.Redis.csproj

@@ -18,6 +18,7 @@

  <ItemGroup>
    <InternalsVisibleTo Include="Aspire.Hosting.Tests" />


We should aim to remove this.

tests/Aspire.Hosting.Tests/ManifestGenerationTests.cs

tests/Aspire.Hosting.Tests/Containers/ContainerResourceBuilderTests.cs

tests/Aspire.Hosting.Redis.Tests/XunitAttributes.cs

davidfowl · 2024-06-12T13:13:19Z

tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs

+public class RedisFunctionalTests
+{
+    [Fact]
+    public async Task VerifyRedisResource()


cc @eerhardt @radical @ReubenBond @mitchdenny thoughts?

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

[InlineData(TestResourceNames.redis)]

[InlineData(TestResourceNames.garnet)]

[InlineData(TestResourceNames.valkey)]

[InlineData(TestResourceNames.sqlserver)]

[InlineData(TestResourceNames.milvus)]

public Task VerifyComponentWorks(TestResourceNames resourceName)

=> RunTestAsync(async () =>

{

_integrationServicesFixture.EnsureAppHasResources(resourceName);

try

{

var response = await _integrationServicesFixture.IntegrationServiceA.HttpGetAsync("http", $"/{resourceName}/verify");

var responseContent = await response.Content.ReadAsStringAsync();

Assert.True(response.IsSuccessStatusCode, responseContent);

}

catch

{

await _integrationServicesFixture.DumpComponentLogsAsync(resourceName, _testOutput);

throw;

}

});

It is definitely simpler than the end-to-end tests.

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.

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?

I don't think it's testing less than the current E2E tests

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() });

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?

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.

tests/Aspire.Hosting.Redis.Tests/Directory.Build.props

tests/Aspire.Hosting.Redis.Tests/Aspire.Hosting.Redis.Tests.csproj

# Conflicts: # Aspire.sln # src/Aspire.Hosting/Aspire.Hosting.csproj # tests/Aspire.Hosting.Containers.Tests/ContainerResourceBuilderTests.cs

Otherwise would try to run it on Windows in CI where Docker is not available

sebastienros · 2024-07-01T15:53:29Z

Added some tests for WithDataVolume() for now. I don't know yet how we can clean these once the test is done.

dotnet-bot · 2024-07-01T18:31:32Z

‼️ Found issues ‼️

Project	Coverage Type	Expected	Actual
Microsoft.Extensions.ServiceDiscovery	Branch	81	79.31 🔻
Microsoft.Extensions.ServiceDiscovery	Line	81	79.01 🔻

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=726269&view=codecoverage-tab

Nothing works anymore, checking the simplest one for conflicts

eerhardt · 2024-07-03T21:41:21Z

@sebastienros - it looks like the tests are still failing

radical · 2024-07-03T21:43:59Z

Aspire.Hosting.Redis.Tests logs show:

could not create the container {"Container": {"name":"redis-mcqtmvnk-64491c08"}, "Reconciliation": 3, "error": "docker command 'CreateContainer' returned with non-zero exit code 1: command output: Stdout: '' Stderr: 'Unable to find image 'redis:7.2' locally\nError response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit\n'"}

radical · 2024-07-03T21:48:23Z

At least that was the case with the first attempt.

I'm not sure about the second one because this shows ##[error]Artifact Logs_Build_Linux_Release already exists for build 728907. for that. That file name needs to append the attempt number.

eerhardt

Maybe we should split this apart and add the Volume/BindMount tests in a separate PR?

eerhardt · 2024-07-03T21:42:12Z

src/Aspire.Hosting.Redis/Aspire.Hosting.Redis.csproj

@@ -22,6 +22,7 @@

  <ItemGroup>
    <InternalsVisibleTo Include="Aspire.Hosting.Tests" />
+    <InternalsVisibleTo Include="Aspire.Hosting.Redis.Tests" />


@sebastienros - can we remove this? What is it needed for?

RedisContainerImageTags
RedisCommanderConfigWriterHook

Are used in these tests and internal.
This is why Aspire.Hosting.Tests was already listed, because these tests are coming from this project (moved to this new Aspire.Hosting.Redis.Tests)

src/Aspire.Hosting/Aspire.Hosting.csproj

eerhardt · 2024-07-03T21:43:40Z

tests/Aspire.Hosting.Containers.Tests/ContainerResourceBuilderTests.cs

+        var redis = builder.AddRedis("redis").WithImageTag("7.1");
+        Assert.Equal("7.1", redis.Resource.Annotations.OfType<ContainerImageAnnotation>().Single().Tag);


Should this be reverted? Why it is using Redis in the core "Hosting.Containers" tests?

eerhardt · 2024-07-03T21:45:13Z

tests/Aspire.Hosting.Redis.Tests/Aspire.Hosting.Redis.Tests.csproj

+    <ProjectReference Include="..\..\src\Aspire.Hosting.AppHost\Aspire.Hosting.AppHost.csproj" IsAspireProjectResource="false" />
+    <ProjectReference Include="..\..\src\Aspire.Hosting.Redis\Aspire.Hosting.Redis.csproj" IsAspireProjectResource="false" />


Suggested change

<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>

Works as you suggest

tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs

tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj

eerhardt · 2024-07-03T21:54:36Z

tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs

+
+        await VerifyDataPersistence(
+            options => options.WithDataVolume(volumeName).WithPersistence(snapshotInterval),
+            async redisClient => await Task.Delay(snapshotInterval + TimeSpan.FromSeconds(1))


This seems like it will be flaky - especially on slow machines.

I am ok to remove it, as long as we agree that the other explicit SAVE call is sufficient to test the functionality since we already have a unit test that ensures the correct --save argument is passed with a custom timespan. This test is only validating that the feature actually works in Redis. Or we can add more that "1 more second after the time that Redis should have saved it".

sebastienros · 2024-07-09T12:37:25Z

tests/Aspire.Hosting.Redis.Tests/AddRedisTests.cs

@@ -169,7 +171,10 @@ public async Task SingleRedisInstanceProducesCorrectRedisHostsVariable(string co

        var commander = builder.Resources.Single(r => r.Name.EndsWith("-commander"));

-        var config = await EnvironmentVariableEvaluator.GetEnvironmentVariablesAsync(commander, DistributedApplicationOperation.Run, TestServiceProvider.Instance);
+        var config = await EnvironmentVariableEvaluator.GetEnvironmentVariablesAsync(


Line too long couldn't read the code.

tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs

sebastienros · 2024-07-09T16:00:04Z

tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs

+                Assert.True(value.IsNull);
+            }
+
+            await app.StopAsync();


This required with AttemptDeleteDockerVolume as otherwise the container is still running until the test is done, and holds a ref to it. Also the two containers exist concurrently without it.

I would expect app.Dispose to call StopAsync but it's not the case.

I would expect app.Dispose to call StopAsync but it's not the case.

I would expect the same thing. @mitchdenny - is this a bug in DistributedApplication?

src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs

eerhardt · 2024-07-09T23:45:00Z

/azp run

azure-pipelines · 2024-07-09T23:45:12Z

Azure Pipelines successfully started running 1 pipeline(s).

mitchdenny · 2024-07-10T12:02:07Z

@sebastienros looks like the restructured tests are failing.

sebastienros · 2024-07-10T12:04:52Z

@mitchdenny are you aware of #4786 ?
This is hitting us hard, we are working on it.

mitchdenny · 2024-07-10T12:10:10Z

I'm aware of that, although I think these test failures aren't related (I could be wrong). If they are then feel free to ignore, and obviously #4786 takes priority.

sebastienros · 2024-07-10T12:16:28Z

I think it's related, last time I had the same issue I asked @radical and he showed me the throttling issue. Also these tests worked earlier when Eric triggered a new build (when the quotas got renewed apparently). But I'll check in the log to see if I can find a confirmation.

sebastienros · 2024-07-10T12:25:08Z

Confirmed in the logs:

fail: Aspire.Hosting.Dcp.dcpctrl.ContainerReconciler[0]
      could not create the container	{"Container": {"name":"redis-fexhgyze-5dedf3af"}, "Reconciliation": 3, "error": "docker command 'CreateContainer' returned with non-zero exit code 1: command output: Stdout: '' Stderr: 'Unable to find image 'redis:7.2' locally\nError response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading:

It's showing up first because it's a job before Helix runs, it's the first tests that try to download docker images. Then the Job is terminated.

Split redis tests into separate test assembly

50a9e63

davidfowl requested review from mitchdenny, eerhardt and joperezr June 11, 2024 21:44

dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label Jun 11, 2024

davidfowl commented Jun 11, 2024

View reviewed changes

src/Aspire.Hosting/Aspire.Hosting.csproj Outdated Show resolved Hide resolved

davidfowl commented Jun 11, 2024

View reviewed changes

joperezr reviewed Jun 11, 2024

View reviewed changes

tests/Aspire.Hosting.Tests/ManifestGenerationTests.cs Outdated Show resolved Hide resolved

joperezr reviewed Jun 11, 2024

View reviewed changes

tests/Aspire.Hosting.Tests/Containers/ContainerResourceBuilderTests.cs Outdated Show resolved Hide resolved

joperezr reviewed Jun 11, 2024

View reviewed changes

tests/Aspire.Hosting.Redis.Tests/XunitAttributes.cs Outdated Show resolved Hide resolved

davidfowl added 2 commits June 11, 2024 15:20

Made some fixes

8d36ff8

Added functional test

c6a1655

davidfowl commented Jun 12, 2024

View reviewed changes

Use the resource name

fbda632

eerhardt reviewed Jun 17, 2024

View reviewed changes

tests/Aspire.Hosting.Redis.Tests/Directory.Build.props Outdated Show resolved Hide resolved

eerhardt reviewed Jun 17, 2024

View reviewed changes

tests/Aspire.Hosting.Redis.Tests/Aspire.Hosting.Redis.Tests.csproj Outdated Show resolved Hide resolved

joperezr mentioned this pull request Jun 24, 2024

Add more test coverage for the WithDataVolume and WithDataBindMount resource extension methods #3185

Closed

sebastienros added 5 commits July 1, 2024 01:15

Merge remote-tracking branch 'origin/main' into davidfowl/redis-tests

3488419

# Conflicts: # Aspire.sln # src/Aspire.Hosting/Aspire.Hosting.csproj # tests/Aspire.Hosting.Containers.Tests/ContainerResourceBuilderTests.cs

PR feedback

98d2f86

Fix documentation

7e9eef0

Mark functional test with RequiresDocker

c8be7b3

Otherwise would try to run it on Windows in CI where Docker is not available

Create AddDataVolume tests for Redis

6462358

sebastienros added 2 commits July 2, 2024 02:54

Add BindMount tests

1394ee8

Add false positive check

7268fe3

sebastienros marked this pull request as ready for review July 3, 2024 05:42

sebastienros added 3 commits July 2, 2024 22:42

Merge remote-tracking branch 'origin/main' into davidfowl/redis-tests

6818dab

Import TestServiceProvider

03e54d3

Debugging functional tests

153f97b

Nothing works anymore, checking the simplest one for conflicts

sebastienros requested a review from eerhardt July 3, 2024 16:17

eerhardt reviewed Jul 3, 2024

View reviewed changes

Apply PR feedback

0859fb6

sebastienros reviewed Jul 9, 2024

View reviewed changes

sebastienros added 2 commits July 9, 2024 06:00

Remove InternalVisibleTo

497e633

Delete volumes after usage

c55aa34

sebastienros reviewed Jul 9, 2024

View reviewed changes

eerhardt reviewed Jul 9, 2024

View reviewed changes

src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs Show resolved Hide resolved

sebastienros added 2 commits July 9, 2024 09:52

Fix erroneous samples

9bc54e9

Move AttemptDeleteDockerVolume to Utils class

69f4ea9

eerhardt approved these changes Jul 9, 2024

View reviewed changes

sebastienros merged commit 3ddc4ca into main Jul 10, 2024
9 checks passed

sebastienros deleted the davidfowl/redis-tests branch July 10, 2024 06:45

Alirexaa mentioned this pull request Jul 12, 2024

Extract Hosting Components tests project #4865

Closed

16 tasks

eerhardt mentioned this pull request Jul 12, 2024

Break up the TestProject to individual projects per Resource/component #4294

Closed

20 tasks

github-actions bot locked and limited conversation to collaborators Aug 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split redis tests into separate test assembly #4468

Split redis tests into separate test assembly #4468

davidfowl commented Jun 11, 2024 •

edited by dotnet-policy-service bot

Loading

davidfowl Jun 11, 2024

davidfowl Jun 12, 2024

eerhardt Jun 17, 2024

mitchdenny Jun 18, 2024

sebastienros Jul 1, 2024

eerhardt Jul 9, 2024

sebastienros commented Jul 1, 2024

dotnet-bot commented Jul 1, 2024

eerhardt commented Jul 3, 2024

radical commented Jul 3, 2024

radical commented Jul 3, 2024

eerhardt left a comment

eerhardt Jul 3, 2024

sebastienros Jul 9, 2024 •

edited

Loading

eerhardt Jul 3, 2024

sebastienros Jul 9, 2024

eerhardt Jul 3, 2024

sebastienros Jul 9, 2024

eerhardt Jul 3, 2024

sebastienros Jul 9, 2024

sebastienros Jul 9, 2024

sebastienros Jul 9, 2024

eerhardt Jul 9, 2024

eerhardt commented Jul 9, 2024

azure-pipelines bot commented Jul 9, 2024

mitchdenny commented Jul 10, 2024

sebastienros commented Jul 10, 2024

mitchdenny commented Jul 10, 2024

sebastienros commented Jul 10, 2024

sebastienros commented Jul 10, 2024

		@@ -18,6 +18,7 @@

		<ItemGroup>
		<InternalsVisibleTo Include="Aspire.Hosting.Tests" />

	[InlineData(TestResourceNames.redis)]
	[InlineData(TestResourceNames.garnet)]
	[InlineData(TestResourceNames.valkey)]
	[InlineData(TestResourceNames.sqlserver)]
	[InlineData(TestResourceNames.milvus)]
	public Task VerifyComponentWorks(TestResourceNames resourceName)
	=> RunTestAsync(async () =>
	{
	_integrationServicesFixture.EnsureAppHasResources(resourceName);
	try
	{
	var response = await _integrationServicesFixture.IntegrationServiceA.HttpGetAsync("http", $"/{resourceName}/verify");
	var responseContent = await response.Content.ReadAsStringAsync();

	Assert.True(response.IsSuccessStatusCode, responseContent);
	}
	catch
	{
	await _integrationServicesFixture.DumpComponentLogsAsync(resourceName, _testOutput);
	throw;
	}
	});

		var redis = builder.AddRedis("redis").WithImageTag("7.1");
		Assert.Equal("7.1", redis.Resource.Annotations.OfType<ContainerImageAnnotation>().Single().Tag);

		<ProjectReference Include="..\..\src\Aspire.Hosting.AppHost\Aspire.Hosting.AppHost.csproj" IsAspireProjectResource="false" />
		<ProjectReference Include="..\..\src\Aspire.Hosting.Redis\Aspire.Hosting.Redis.csproj" IsAspireProjectResource="false" />

Split redis tests into separate test assembly #4468

Split redis tests into separate test assembly #4468

Conversation

davidfowl commented Jun 11, 2024 • edited by dotnet-policy-service bot Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros commented Jul 1, 2024

dotnet-bot commented Jul 1, 2024

eerhardt commented Jul 3, 2024

radical commented Jul 3, 2024

radical commented Jul 3, 2024

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Jul 9, 2024

azure-pipelines bot commented Jul 9, 2024

mitchdenny commented Jul 10, 2024

sebastienros commented Jul 10, 2024

mitchdenny commented Jul 10, 2024

sebastienros commented Jul 10, 2024

sebastienros commented Jul 10, 2024

davidfowl commented Jun 11, 2024 •

edited by dotnet-policy-service bot

Loading

sebastienros Jul 9, 2024 •

edited

Loading