Conversation
Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/162f2c71-6716-4aa4-99bd-74a2a87f8e9a Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/162f2c71-6716-4aa4-99bd-74a2a87f8e9a Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1779 +/- ##
==========================================
+ Coverage 61.18% 62.37% +1.18%
==========================================
Files 285 285
Lines 8405 8421 +16
Branches 974 980 +6
==========================================
+ Hits 5143 5253 +110
+ Misses 3026 2935 -91
+ Partials 236 233 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WhitWaldo
left a comment
There was a problem hiding this comment.
Regarding the tests - please look for opportunities to add more and richer unit and integration tests for the Dapr.Actors and Dapr.Actors.AspNetCore projects (e.g. can we validate that the state manager caching mechanism works as expected)? When writing the tests, both what you've written and what you're going to write, please ensure that you're testing for correctness, not to simply validates that what is already there will work as written.
| /// Initializes a new instance of <see cref="ReentrantActor"/>. | ||
| /// </summary> | ||
| /// <param name="host">The actor host provided by the Dapr runtime.</param> | ||
| public ReentrantActor(ActorHost host) : base(host) |
There was a problem hiding this comment.
Prefer to use primary constructors where possible
There was a problem hiding this comment.
Converted all actor implementations (ReentrantActor, RegressionActor, ReminderActor, StateActor, AdvancedStateActor, ExceptionActor, SerializationActor, WeaklyTypedTestingActor, TimerActor) to use C# 12 primary constructors in commit 39f1e5c.
| /// Initializes a new instance of <see cref="ReminderActor"/>. | ||
| /// </summary> | ||
| /// <param name="host">The actor host provided by the Dapr runtime.</param> | ||
| public ReminderActor(ActorHost host) : base(host) |
There was a problem hiding this comment.
Converted ReminderActor to use a primary constructor in commit 39f1e5c.
…, multi-line XML docs, richer state tests Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/e6087c47-d287-4088-8a11-c8fa6e169c36 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Added
|
…ntime, AspNetCore and integration test coverage Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/9e596730-9751-40b6-a25c-5e30c0887ef9 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/9e596730-9751-40b6-a25c-5e30c0887ef9 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
|
@copilot I'm seeing failures across the board of all the actors-based integration test projects you assembled here - please fix them all. |
…llectionExtensions The ConfigureActorOptions method was calling DaprDefaults.GetDefaultHttpEndpoint() without passing the IConfiguration instance, causing all Dapr.IntegrationTest.Actors integration tests to fail with 'Connection refused (localhost:3500)'. The DaprTestApplicationBuilder puts the correct Dapr HTTP port into in-memory IConfiguration (DAPR_HTTP_ENDPOINT), but GetDefaultHttpEndpoint() without configuration falls back to environment variables only, getting the default 3500. Fix: pass configuration to GetDefaultHttpEndpoint(configuration) to ensure IConfiguration values take precedence, consistent with DaprApiToken handling. Also add a new unit test AddActors_HttpEndpointFromConfiguration_IsReflectedInProxyFactory that covers this fallback path. Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/f04fa157-9f33-4618-aa84-f453df44f1f0 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/f04fa157-9f33-4618-aa84-f453df44f1f0 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Fixed in commit Root cause: The fix is a one-line change in // before
: DaprDefaults.GetDefaultHttpEndpoint();
// after
: DaprDefaults.GetDefaultHttpEndpoint(configuration);This is the same pattern already used for Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…duler disposed before testApp In all 9 actor integration test files, CreateTestAppAsync used `await using var environment = ...` which disposed the DaprTestEnvironment (placement, scheduler, Redis) the moment the helper method returned — well before the test had finished using the running app. The Dapr sidecar could not reach placement, so actor types were never registered, causing WaitForActorRuntimeAsync to retry for the full 120-second outer timeout before failing. Fix: introduce ActorTestContext (IAsyncDisposable) that holds both DaprTestEnvironment and DaprTestApplication, disposing the app first then the environment. CreateTestAppAsync now returns ActorTestContext instead of DaprTestApplication so the environment's lifetime is tied to the test rather than to the helper-method scope. Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/b7098af2-cd6d-4f5a-bc74-65e98df2f6d2 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/b7098af2-cd6d-4f5a-bc74-65e98df2f6d2 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Fixed in commit Root cause: In all 9 Fix: introduced |
|
@copilot I'm seeing a lot of red in the latest build. Can you give it another pass? |
…n comparison, reminder not-found Three root causes addressed: 1. ActorStateTTL failures: DaprdContainer lacked --config flag support. Added optional configFilePath parameter to DaprdContainer, DaprConfigFilePath property to BaseHarness, and ActorHarness.OnInitializeAsync now writes actor-config.yaml enabling ActorStateTTL and passes its container path to daprd via -config. 2. Serialization test failure: Assert.Equal(payload.Value.GetRawText(), result.Value.GetRawText()) failed because WriteIndented:true in the actor's custom JsonSerializerOptions reformats the round-tripped JsonElement with whitespace. Fix: compare via JsonSerializer.Serialize() which normalises to compact JSON regardless of the source representation. 3. Reminder not-found failure: Dapr 1.12+ returns an error (not 500/null) when GetReminder is called on a non-existent reminder. ReminderActor.GetReminder() now catches DaprApiException and returns "null", matching what the test polls for before/after the reminder runs. Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/def146a4-f88a-43e2-b50f-f6138e2803b7 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
…atch to not-found only Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/def146a4-f88a-43e2-b50f-f6138e2803b7 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Fixed in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot Still seeing test failures! |
All actor-related test failures are resolved as of commit The only remaining failure is |
Description
Adds comprehensive unit and integration test coverage for the
Dapr.ActorsandDapr.Actors.AspNetCoreprojects, fixes a production bug inActorsServiceCollectionExtensionswhereIConfigurationwas not passed toGetDefaultHttpEndpoint(), fixes 100-second timeout failures in the integration test readiness probe, fixes a critical environment lifetime scoping bug that caused all actor integration tests to fail, and resolves additional integration test failures related to theActorStateTTLfeature flag, JSON serialization comparison, Dapr 1.12+ reminder API behavior, andDaprdContainerconfiguration support.Changes Made
Bug Fix —
ActorsServiceCollectionExtensionsConfigureActorOptionswas callingDaprDefaults.GetDefaultHttpEndpoint()without passing theIConfigurationinstance. TheDaprTestApplicationBuilderinjects the correct Dapr sidecar port into in-memoryIConfiguration(keyDAPR_HTTP_ENDPOINT), but withoutconfigurationthe method fell back to environment variables only, defaulting tolocalhost:3500.Bug Fix —
WaitForActorRuntimeAsync100-second timeoutWaitForActorRuntimeAsynconly caughtDaprApiException. When the Dapr sidecar accepts the TCP connection but is still acquiring its placement token, the actor method call hangs for the full HttpClient default timeout (100 seconds) and then throwsTaskCanceledException, which propagated straight out of the retry loop and failed the test immediately.The fix:
CancellationToken cancellationToken = defaulttoIPingActor.Ping()and all actor implementations so a short-lived token is wired through to the underlying HTTP request.WaitForActorRuntimeAsyncnow creates a per-attemptCancellationTokenSourcecapped at 5 seconds, so a hung placement-registration request is cancelled and retried quickly instead of stalling for 100 s.OperationCanceledException(when not from the outer token) andHttpRequestExceptionin the retry loop so all transient sidecar startup conditions are retried rather than failing the test.Bug Fix —
DaprTestEnvironmentlifetime scoping in actor integration testsAll 9
CreateTestAppAsynchelper methods usedawait using var environment = ..., which disposed theDaprTestEnvironment(placement service, scheduler, Redis) the moment the helper method returned — before any test assertions ran. With placement gone, the Dapr sidecar could never register actor types, soWaitForActorRuntimeAsyncretried for the full 120-second outer timeout before failing.The fix introduces
ActorTestContext— a thinIAsyncDisposablewrapper that holds both theDaprTestEnvironmentand theDaprTestApplication, disposing the app first then the environment. All 9 test files'CreateTestAppAsyncnow returnActorTestContextso the environment's lifetime is tied to the test rather than the helper-method scope.Bug Fix —
ActorStateTTLfeature not enabled in daprdActor state TTL tests failed with
ttlInSeconds is not supported without the "ActorStateTTL" feature enabledbecause the daprd container was never started with a Dapr configuration file enabling this feature gate.The fix:
configFilePathparameter toDaprdContainerso daprd can be launched with-config <path>.DaprConfigFilePathprotected property toBaseHarnessthat is forwarded toDaprdContaineron startup.ActorHarness.OnInitializeAsyncnow writes anactor-config.yaml(DaprConfigurationresource withActorStateTTL: true) to the components directory and setsDaprConfigFilePath = "/components/actor-config.yaml".Bug Fix — Serialization test
GetRawText()whitespace mismatchActorCanSupportCustomSerializercomparedpayload.Value.GetRawText()withresult.Value.GetRawText(). Because the actor's customJsonSerializerOptionssetsWriteIndented: true, the round-trippedJsonElementis formatted with indentation, producing a different raw string even though the values are equal.Fixed by replacing the raw-text comparison with
Assert.Equal(JsonSerializer.Serialize(payload.Value), JsonSerializer.Serialize(result.Value)), which normalises both to compact JSON regardless of their source representation.Bug Fix — Reminder
GetReminder()throws on Dapr 1.12+Dapr 1.12+ returns a 404 error (not a 500 that the SDK silently mapped to
null) whenGetReminderis called on a non-existent reminder. This causedActorCanStartAndStopAndGetReminderto fail withactor reminder not found: test-reminderduring the polling loop after the reminder had self-cancelled.ReminderActor.GetReminder()now catchesDaprApiExceptionwhenex.Message.Contains("not found")and returns"null", matching the expected pre-registered and post-stopped state that the test polls for.New Tests
ActorStateManagerunit tests (Dapr.Actors.Test):GetOrAddStateAsync,AddOrUpdateStateAsync,TryAddStateAsync,ContainsStateAsync,TryRemoveStateAsync,ClearCacheAsync,SaveStateAsynccorrectness, andSetStateContextreentrancy — all designed to verify correctness of caching behaviour rather than just absence of exceptions.DaprStateProviderunit tests:SaveStateAsyncwithRemove/Updatechange kinds, TTL boundary conditions.ActorRuntime/ActorManagergap tests: unknown actor type/method dispatch, empty entities serialization, per-actor reentrancy config, and default option values not serialized.Dapr.Actors.AspNetCore.Testtests:HttpEndpointandDaprApiTokenpropagation (including the newIConfigurationfallback path), route-table tests, andDependencyInjectionActorActivatorwith unregistered type.Dapr.Actors.AspNetCore.IntegrationTesttests: non-existent actor type error handling, concurrent calls for the same actor ID.Dapr.IntegrationTest.Actorsproject: full end-to-end integration tests with a live Dapr sidecar via Testcontainers, covering state management (TTL, multi-key isolation, cache read-through), reentrancy, reminders, timers, serialization, weakly-typed actors, regression, and exception handling.Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: