feat: Add Aspire dashboard module#1194
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
HofmeisterAn
left a comment
There was a problem hiding this comment.
Thanks for your PR. Overall, it looks good and aligns well with our module approach. Thank you for following the repository standards. However, there are a few things we need to address before we can merge the PR. Nothing major. If you have any further questions, need help, or would like to discuss any topics, I am happy to help.
|
Sorry, long time no see. I plan to continue working on this one |
@HofmeisterAn please take a look. Thank you |
4900ecd to
8fa5f1b
Compare
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an Aspire Dashboard module with builder, configuration, container, connection-string support, solution wiring, test project, integration test, and docs. Also updates Azurite constructor documentation links. ChangesAspire Dashboard Module
Azurite Builder Docs
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AspireDashboardBuilder
participant AspireDashboardContainer
participant HttpClient
Caller->>AspireDashboardBuilder: new AspireDashboardBuilder(image)
Caller->>AspireDashboardBuilder: Build()
AspireDashboardBuilder->>AspireDashboardContainer: new AspireDashboardContainer(configuration)
Caller->>AspireDashboardContainer: GetDashboardAddress()
HttpClient->>AspireDashboardContainer: GET /
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/Testcontainers.AspireDashboard/AspireDashboardBuilder.cs (1)
12-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd XML doc comments to the public constants.
These constants are part of the
[PublicAPI]surface and are referenced by consumers (e.g. the test'sWithPortBinding). Documenting them keeps the public API consistent with the rest of the type and avoids missing-doc warnings if XML docs are enforced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Testcontainers.AspireDashboard/AspireDashboardBuilder.cs` around lines 12 - 17, The public constants in AspireDashboardBuilder are missing XML documentation, which leaves the [PublicAPI] surface inconsistent. Add XML doc comments for AspireDashboardImage, AspireDashboardFrontendPort, and AspireDashboardOtlpPort in AspireDashboardBuilder so the public API is documented consistently and avoids missing-doc warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/modules/aspire-dashboard.md`:
- Around line 8-20: The Aspire Dashboard sample in the documentation is
inconsistent with the guidance: it demonstrates an HTTP/unsecured transport
scenario but calls AllowUnsecuredTransport with false, and it also claims port
customization without showing a different port. Update the example around
AspireDashboardBuilder and AspireDashboardContainerTest so the transport setting
matches the HTTP usage and the WithPortBinding call clearly demonstrates
changing the port to a non-default value.
In `@src/Testcontainers.AspireDashboard/AspireDashboardContainer.cs`:
- Around line 16-33: Move the dashboard and OTLP URL logging out of the
AspireDashboardContainer constructor/setup path and into the Started event
handler so GetMappedPublicPort is only called after the container is running.
Update the Started callback in AspireDashboardContainer to log both URLs there,
following the pattern used by CouchbaseContainer, and remove the premature
logging before startup.
In `@tests/Testcontainers.AspireDashboard.Tests/AspireDashboardContainerTest.cs`:
- Around line 8-11: The AspireDashboardContainerTest setup is hard-coding the
host port in the WithPortBinding call, which can cause collisions and flaky
runs. Update the container configuration in AspireDashboardContainerTest to use
a random host-port binding instead of the fixed
AspireDashboardBuilder.AspireDashboardFrontendPort value, and rely on the
container API to read the mapped port when the test needs it.
In
`@tests/Testcontainers.AspireDashboard.Tests/Testcontainers.AspireDashboard.Tests.csproj`:
- Line 12: Remove the unused ArangoDBNetStandard package reference from the
Testcontainers.AspireDashboard.Tests project file, since this test project does
not use any ArangoDB APIs. Update the
Testcontainers.AspireDashboard.Tests.csproj by deleting the unnecessary
PackageReference entry and keep the rest of the test dependencies unchanged.
---
Nitpick comments:
In `@src/Testcontainers.AspireDashboard/AspireDashboardBuilder.cs`:
- Around line 12-17: The public constants in AspireDashboardBuilder are missing
XML documentation, which leaves the [PublicAPI] surface inconsistent. Add XML
doc comments for AspireDashboardImage, AspireDashboardFrontendPort, and
AspireDashboardOtlpPort in AspireDashboardBuilder so the public API is
documented consistently and avoids missing-doc warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 620ded7b-8268-48f1-8321-4f55e85c5ea1
📒 Files selected for processing (13)
Testcontainers.slndocs/modules/aspire-dashboard.mdmkdocs.ymlsrc/Testcontainers.AspireDashboard/.editorconfigsrc/Testcontainers.AspireDashboard/AspireDashboardBuilder.cssrc/Testcontainers.AspireDashboard/AspireDashboardConfiguration.cssrc/Testcontainers.AspireDashboard/AspireDashboardContainer.cssrc/Testcontainers.AspireDashboard/Testcontainers.AspireDashboard.csprojsrc/Testcontainers.AspireDashboard/Usings.cstests/Testcontainers.AspireDashboard.Tests/.editorconfigtests/Testcontainers.AspireDashboard.Tests/AspireDashboardContainerTest.cstests/Testcontainers.AspireDashboard.Tests/Testcontainers.AspireDashboard.Tests.csprojtests/Testcontainers.AspireDashboard.Tests/Usings.cs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Testcontainers.AspireDashboard/AspireDashboardBuilder.cs`:
- Around line 29-35: The XML docs on AspireDashboardBuilder still show an
Azurite example in the <param name="image"> documentation, which is misleading
for this dashboard builder. Update the example in the AspireDashboardBuilder
documentation to reference a valid Aspire Dashboard image instead of the copied
Azurite image, and keep the surrounding remarks about the Docker image tag
source intact.
- Around line 12-14: The public Aspire Dashboard OTLP constants and helpers
currently use the misspelled “Oltp” naming, so rename them to “Otlp” throughout
the API before release. Update the constants in AspireDashboardBuilder and any
mirrored public helper names in AspireDashboardContainer so the public surface
consistently exposes the correct OTLP spelling. Ensure all references and usages
are renamed together to avoid leaving any typo in the API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 091107aa-5893-445d-9424-8483442dcbbe
📒 Files selected for processing (9)
src/Testcontainers.AspireDashboard/AspireDashboardBuilder.cssrc/Testcontainers.AspireDashboard/AspireDashboardConfiguration.cssrc/Testcontainers.AspireDashboard/AspireDashboardContainer.cssrc/Testcontainers.AspireDashboard/Testcontainers.AspireDashboard.csprojsrc/Testcontainers.AspireDashboard/Usings.cssrc/Testcontainers.Azurite/AzuriteBuilder.cstests/Testcontainers.AspireDashboard.Tests/AspireDashboardContainerTest.cstests/Testcontainers.AspireDashboard.Tests/Dockerfiletests/Testcontainers.AspireDashboard.Tests/Testcontainers.AspireDashboard.Tests.csproj
✅ Files skipped from review due to trivial changes (2)
- src/Testcontainers.AspireDashboard/Usings.cs
- src/Testcontainers.Azurite/AzuriteBuilder.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Testcontainers.AspireDashboard.Tests/AspireDashboardContainerTest.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Testcontainers.AspireDashboard.Tests/AspireDashboardContainerTest.cs (1)
87-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBrittle substring assertions on serialized JSON.
"\"totalCount\":1"depends on the dashboard serializing without whitespace; a formatting change would break this test for non-functional reasons. Consider parsing the JSON and asserting on typed values instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Testcontainers.AspireDashboard.Tests/AspireDashboardContainerTest.cs` around lines 87 - 89, The test currently relies on brittle substring checks against serialized JSON, so it can fail on harmless formatting changes. Update the assertions in AspireDashboardContainerTest to parse spansJson as JSON and verify the typed values, using the existing _serviceName and _spanName checks along with the totalCount field from the parsed object instead of matching the raw string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Testcontainers.AspireDashboard.Tests/AspireDashboardContainerTest.cs`:
- Around line 83-89: The span assertion in AspireDashboardContainerTest is
racing the dashboard’s asynchronous ingestion/indexing, so the immediate GET to
/api/telemetry/spans can return before the span is visible. Update the test flow
around the tracer provider disposal and the spans query to poll for the expected
span with a short timeout and retry interval until totalCount reflects the
ingested span, then keep the existing assertions on _serviceName and _spanName.
---
Nitpick comments:
In `@tests/Testcontainers.AspireDashboard.Tests/AspireDashboardContainerTest.cs`:
- Around line 87-89: The test currently relies on brittle substring checks
against serialized JSON, so it can fail on harmless formatting changes. Update
the assertions in AspireDashboardContainerTest to parse spansJson as JSON and
verify the typed values, using the existing _serviceName and _spanName checks
along with the totalCount field from the parsed object instead of matching the
raw string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 331ae40f-6270-45f1-804d-3e47fbfa7aa9
📒 Files selected for processing (5)
Directory.Packages.propsmkdocs.ymltests/Testcontainers.AspireDashboard.Tests/AspireDashboardContainerTest.cstests/Testcontainers.AspireDashboard.Tests/Testcontainers.AspireDashboard.Tests.csprojtests/Testcontainers.AspireDashboard.Tests/Usings.cs
✅ Files skipped from review due to trivial changes (2)
- tests/Testcontainers.AspireDashboard.Tests/Usings.cs
- Directory.Packages.props
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Testcontainers.AspireDashboard.Tests/Testcontainers.AspireDashboard.Tests.csproj
What does this PR do?
Adds Aspire Dashboard support.
Why is it important?
As discussed in #1190, it would be great to have this Testcontainer.
Summary by CodeRabbit