Skip to content

test(dashboard): cover hosting and routing behavior - #10885

Merged
ReubenBond merged 4 commits into
dotnet:mainfrom
ReubenBond:rb-test-hosting-close-package-coverage-gaps
Aug 28, 2026
Merged

ReubenBond merged 4 commits into
dotnet:mainfrom
ReubenBond:rb-test-hosting-close-package-coverage-gaps

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Aug 28, 2026

Copy link
Copy Markdown
Member

Part of #10865.

Dashboard was the largest independently mergeable long-tail package gap in the canonical merged report at 42.24% line coverage. This change adds behavior-focused tests for dashboard hosting and dependency injection, configured options, endpoint routing and authorization, API forwarding and failure responses, host lifecycle behavior, static asset caching/compression, logging, and TimeSpan JSON conversion.

The focused Dashboard report moves from 757/1,792 lines (42.24%) to 1,167/1,792 lines (65.12%), covering 410 additional lines (+22.88 percentage points). The primary ServiceCollectionExtensions class moves from 18.18% to 100% line coverage, while the suite grows from 38 to 129 passing tests without production-code changes.

Fixes: #10865

Microsoft Reviewers: Open in CodeFlow

Copilot AI lite review requested due to automatic review settings August 28, 2026 01:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

Review tier: Lite
Findings: 1 High severity · 1 Medium severity

New issues introduced by this change (2)
Severity Finding
High severity test/​Orleans.Dashboard.Tests/​Orleans.Dashboard.UnitTests/​TimeSpanConverterTests.csJsonSerializer.Deserialize wraps exceptions thrown by custom converters (including…
Medium severity test/​Orleans.Dashboard.Tests/​Orleans.Dashboard.UnitTests/​EmbeddedAssetTests.csEmbeddedAssetProvider only serves gzip when a precompressed representation exists and it is…
What changed in this PR

Adds a comprehensive behavior-focused test suite for the Orleans.Dashboard package to close a major coverage gap (per #10865) without changing production code. The new tests validate service registration/DI wiring, endpoint routing/authorization and failure responses, host lifecycle behavior, embedded static assets (caching/compression), logging fanout, and TimeSpan JSON conversion.

Changes:

  • Add unit tests for Dashboard DI/service registration and options composition (silo + client builders).
  • Add extensive endpoint routing tests covering static assets, API forwarding, authorization, redirects, and error/failure responses.
  • Add tests for Dashboard host lifecycle/telemetry disposal, Dashboard logger behavior, and TimeSpan JSON converter behavior; expand embedded asset provider assertions.
File Description
test/​Orleans.Dashboard.Tests/​Orleans.Dashboard.UnitTests/​TimeSpanConverterTests.cs Adds TimeSpanConverter serialization/deserialization tests (format, error paths).
test/​Orleans.Dashboard.Tests/​Orleans.Dashboard.UnitTests/​ServiceCollectionExtensionsTests.cs Adds DI graph and registration/lifetime tests for AddDashboard and core services.
test/​Orleans.Dashboard.Tests/​Orleans.Dashboard.UnitTests/​ServiceCollectionExtensionsRoutingTests.cs Adds routing/authorization/API forwarding/static asset behavior tests for MapOrleansDashboard.
test/​Orleans.Dashboard.Tests/​Orleans.Dashboard.UnitTests/​Implementation/​DashboardLoggerTests.cs Adds tests for DashboardLogger subscriber behavior and formatting.
test/​Orleans.Dashboard.Tests/​Orleans.Dashboard.UnitTests/​EmbeddedAssetTests.cs Strengthens embedded asset provider tests to assert headers/body/caching/compression behavior.
test/​Orleans.Dashboard.Tests/​Orleans.Dashboard.UnitTests/​DashboardOptionsTests.cs Adds tests for default options and option configuration via client/silo builders.
test/​Orleans.Dashboard.Tests/​Orleans.Dashboard.UnitTests/​DashboardHostTests.cs Adds tests for DashboardHost activation warnings, lifecycle behavior, and telemetry disposal safety.
Suppressed comments (1)

test/Orleans.Dashboard.Tests/Orleans.Dashboard.UnitTests/TimeSpanConverterTests.cs:88

  • JsonSerializer.Deserialize<TimeSpan>("null", ...) will surface a JsonException (potentially with an ArgumentNullException inner exception) rather than throwing ArgumentNullException directly. As written, this test is likely to fail on modern System.Text.Json behavior.
    public void Read_NullToken_ThrowsArgumentNullException()
    {
        var exception = Assert.Throws<ArgumentNullException>(
            () => JsonSerializer.Deserialize<TimeSpan>("null", SerializerOptions));


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Code coverage

78.84% line coverage - 99,816 / 126,605 lines

Coverage details

Copilot AI review requested due to automatic review settings August 28, 2026 04:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

Review tier: Lite
Findings: None

Issues resolved since last review (2)
Severity Finding
Medium severity test/​Orleans.Dashboard.Tests/​Orleans.Dashboard.UnitTests/​EmbeddedAssetTests.csEmbeddedAssetProvider only serves gzip when a precompressed representation exists and it is… View resolved comment
High severity test/​Orleans.Dashboard.Tests/​Orleans.Dashboard.UnitTests/​TimeSpanConverterTests.csJsonSerializer.Deserialize wraps exceptions thrown by custom converters (including… View resolved comment
Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

test/Orleans.Dashboard.Tests/Orleans.Dashboard.UnitTests/ServiceCollectionExtensionsRoutingTests.cs:118

  • The Cache-Control header value ordering is not a stable contract (it depends on CacheControlHeaderValue.ToString()), so asserting the exact string "no-store, no-cache" makes this test brittle across framework/library versions. Prefer parsing the header and asserting the NoStore/NoCache directives instead.
        Assert.Equal(StatusCodes.Status200OK, response.StatusCode);
        Assert.Equal("text/css", response.ContentType);
        Assert.Equal(expectedBody.Length, response.ContentLength);
        Assert.Equal("no-store, no-cache", response.Headers.CacheControl.ToString());
        Assert.True(EntityTagHeaderValue.TryParse(response.Headers.ETag.ToString(), out var entityTag));
        Assert.Equal(expectedBody, response.Body);

test/Orleans.Dashboard.Tests/Orleans.Dashboard.UnitTests/DashboardHostTests.cs:163

  • This test disposes the DashboardHost twice (once in the try block and again in finally). IDisposable does not guarantee idempotency, and double-dispose can mask regressions (eg, disposing the MeterProvider more than once). Track whether Dispose was called and only call it from finally if it hasn’t already been invoked.
            host.Dispose();

            Assert.Equal(1, trackingProvider.DisposeCalls);
        }
        finally

Copilot AI review requested due to automatic review settings August 28, 2026 04:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

Review tier: Lite
Findings: None

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

test/Orleans.Dashboard.Tests/Orleans.Dashboard.UnitTests/TimeSpanConverterTests.cs:80

  • This test asserts a framework-generated JsonException.Message substring from System.Text.Json. That message text is not part of the converter’s contract and can change between .NET versions, making the test brittle. Prefer asserting stable properties (exception type + Path) and that the message is non-empty.
    public void Read_NonStringToken_ThrowsJsonException(string json)
    {
        var exception = Assert.Throws<JsonException>(
            () => JsonSerializer.Deserialize<TimeSpan>(json, SerializerOptions));

        Assert.Equal("$", exception.Path);
        Assert.Contains(
            "The JSON value could not be converted to System.TimeSpan.",
            exception.Message,
            StringComparison.Ordinal);

test/Orleans.Dashboard.Tests/Orleans.Dashboard.UnitTests/ServiceCollectionExtensionsRoutingTests.cs:571

  • This asserts the exact set of ProblemDetails properties. The output can include additional fields (eg, traceId/extensions) depending on ASP.NET Core defaults and registered services, so asserting the complete property list is brittle. It’s usually enough to assert the status code/content-type and the specific fields you care about.

This issue also appears on line 572 of the same file.

        Assert.Equal(
            ["detail", "status", "title", "type"],
            root.EnumerateObject()
                .Select(property => property.Name)
                .OrderBy(static name => name, StringComparer.Ordinal));

test/Orleans.Dashboard.Tests/Orleans.Dashboard.UnitTests/ServiceCollectionExtensionsRoutingTests.cs:574

  • This test pins the ProblemDetails type value to a specific RFC URL which is produced by the framework, not the dashboard code. That constant can change between ASP.NET Core versions; consider only asserting that type is present and non-empty.
        Assert.Equal(
            "https://tools.ietf.org/html/rfc9110#section-15.5.4",
            root.GetProperty("type").GetString());

Copilot AI review requested due to automatic review settings August 28, 2026 05:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

Review tier: Lite
Findings: None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(hosting): close long-tail package coverage gaps

2 participants