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

Ensure all integration have at least one test for manifest #115

Open
Alirexaa opened this issue Oct 17, 2024 · 5 comments
Open

Ensure all integration have at least one test for manifest #115

Alirexaa opened this issue Oct 17, 2024 · 5 comments
Labels

Comments

@Alirexaa
Copy link
Member

Example in aspire repo:
https://github.com/dotnet/aspire/blob/4c91f09e9c7f84e73eca6932f550475309155abd/tests/Aspire.Hosting.Redis.Tests/AddRedisTests.cs#L96-L120

    [Fact]
    public async Task VerifyManifest()
    {
        using var builder = TestDistributedApplicationBuilder.Create();
        var redis = builder.AddRedis("redis");


        var manifest = await ManifestUtils.GetManifest(redis.Resource);


        var expectedManifest = $$"""
            {
              "type": "container.v0",
              "connectionString": "{redis.bindings.tcp.host}:{redis.bindings.tcp.port}",
              "image": "{{RedisContainerImageTags.Registry}}/{{RedisContainerImageTags.Image}}:{{RedisContainerImageTags.Tag}}",
              "bindings": {
                "tcp": {
                  "scheme": "tcp",
                  "protocol": "tcp",
                  "transport": "tcp",
                  "targetPort": 6379
                }
              }
            }
            """;
        Assert.Equal(expectedManifest, manifest.ToString());
    }

We need ManifestUtils to generate manifest file.

@aaronpowell
Copy link
Member

How should we handle integrations that aren't for deployment, like the Node.js extensions or Static Web Apps? Do we want to validate that they generate nothing?

@Alirexaa
Copy link
Member Author

Alirexaa commented Oct 21, 2024

How should we handle integrations that aren't for deployment, like the Node.js extensions or Static Web Apps? Do we want to validate that they generate nothing?

I'm not sure about those and I did not see any test about the specific resources in dotnet/aspire to ensure that they generate nothing.
We could add tests about these scenarios to ensure that ExcludeFromManifest is never removed accidentally.

I looked at Node.js extensions and did not see any checking about that ExecuationContext being in run mode. It seems we have an issue with that right now. If we use these extensions only in development, we should have to check that ExecuationContext is in run mode.
And I think it's better to change AddViteApp to WithRunViteApp.

@aaronpowell
Copy link
Member

I looked at Node.js extensions and did not see any checking about that ExecuationContext being in run mode. It seems we have an issue with that right now. If we use these extensions only in development, we should have to check that ExecuationContext is in run mode.
And I think it's better to change AddViteApp to WithRunViteApp.

That's a very good point, I'll add an issue tracking it.

@Alirexaa
Copy link
Member Author

@aaronpowell
I can add ManifestUtil to generate manifest that we need for tests.

public sealed class ManifestUtils
{
    public static async Task<JsonNode> GetManifest(IResource resource, string? manifestDirectory = null)
    {
        var node = await GetManifestOrNull(resource, manifestDirectory);
        Assert.NotNull(node);
        return node;
    }

    public static async Task<JsonNode?> GetManifestOrNull(IResource resource, string? manifestDirectory = null)
    {
        manifestDirectory ??= Environment.CurrentDirectory;

        using var ms = new MemoryStream();
        var writer = new Utf8JsonWriter(ms);
        var executionContext = new DistributedApplicationExecutionContext(DistributedApplicationOperation.Publish);
        writer.WriteStartObject();
        var context = new ManifestPublishingContext(executionContext, Path.Combine(manifestDirectory, "manifest.json"), writer);
        await WriteResourceAsync(context, resource);
        writer.WriteEndObject();
        writer.Flush();
        ms.Position = 0;
        var obj = JsonNode.Parse(ms);
        Assert.NotNull(obj);
        var resourceNode = obj[resource.Name];
        return resourceNode;
    }

    [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "WriteResourceAsync")]
    private static extern Task WriteResourceAsync(ManifestPublishingContext context, IResource resource);
}

The only thing we should consider is UnsafeAccessor in this code.
FYI, the aws repo uses this code

https://github.com/aws/integrations-on-dotnet-aspire-for-aws/blob/main/tests/Aspire.Hosting.AWS.Tests/ManifestUtils.cs

@aaronpowell
Copy link
Member

That's probably the safest approach. While yes, using UnsafeAccessor isn't ideal, it's slightly less error prone that us writing our own reflection code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants