Fix Flaky Integration Tests + Migrate some tests to Test Containers#1754
Fix Flaky Integration Tests + Migrate some tests to Test Containers#1754siri-varma wants to merge 69 commits into
Conversation
Port the BookTrip compensation (Saga) workflow from the plain Java examples into the Spring Boot workflow patterns module, adding @Component-annotated activities and a /wfp/compensation REST endpoint. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Spec for migrating 13 sdk-tests integration tests from the dapr-run based BaseIT/DaprRun/AppRun harness to Testcontainers via DaprContainer. Captures architecture, startup ordering, per-IT plan, CI changes, and the 8 non-migrated ITs that stay on DaprRun. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
- Move ActorStateIT out of migration list (it's a sidecar-restart test). Migration count: 12 logical ITs / 13 files (9 non-migrated). - Add D9: per-class @BeforeAll lifecycle for all migrated ITs, with TracingIT mitigation (per-test trace IDs). - Add D10: each migrated IT subclass owns its own static DaprContainer + AppRun fields; BaseContainerIT provides helpers only, no fields. - Reconcile IT counts throughout (12 logical, 13 files migrated, 9 non-migrated, 22 total). Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
19-task plan covering: SharedTestInfra + BaseContainerIT foundation, AppRun port-override overload, 13 IT migrations (5 easy + 4 actor + 2 method-invoke + 2 tracing), CI compose-up trim, and push/observe. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
- Define final startAppAndAttach signature (returning DaprAndApp record) directly in Task 3 instead of refactoring it in Task 9. - Drop the startApp tombstone that threw UnsupportedOperationException. - Drop the unused newActorClient(dapr, ResiliencyOptions) overload — no migrated actor IT calls it (ActorSdkResiliencyIT is non-migrated). - Fix Task 15's Configuration constructor: it takes 3 args (name, tracing, appHttpPipeline); pass null for the last. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Provides a JVM-wide Network and lazy Redis container shared across all migrated integration tests. Uses withReuse(true) for dev-loop speed. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Lets BaseContainerIT point the spawned app subprocess at a Testcontainer DaprContainer's mapped HTTP/gRPC ports. Existing callers untouched. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
- Add Apache 2.0 license header to new files (matches package convention). - Change AppRun's new constructor overrides from Integer to int to remove NPE risk on a package-private API. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
BaseContainerIT lives in io.dapr.it.containers and Java packages are not hierarchical, so the constructor needs public visibility to be callable from the sub-package. Caught during Task 3 implementation. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Provides daprBuilder, startAppAndAttach (returning DaprAndApp record), newDaprClient(dapr), Component factories, and @afterall cleanup. Each subclass owns its own static DaprContainer + AppRun fields (D10 from the spec). Smoke test boots a no-component DaprContainer to verify the helper plumbing end-to-end. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Document the single-thread JUnit lifecycle assumption on the cleanup deques, clarify withReusablePlacement vs SharedTestInfra's withReuse, and distinguish the two deferStop overloads. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
dc20276 to
280c572
Compare
Boots Dapr via DaprContainer with secretstores.local.file pointing at a JSON payload generated in-memory and injected via withCopyToContainer (Transferable.of). No host file involvement. Drops initSecretFile/clearSecretFile/LOCAL_SECRET_FILE_PATH — workarounds for the old DaprRun harness writing to a shared host file. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Replaces double-brace HashMap initialization with Map.of to set a cleaner template for the 12 downstream IT migrations. Drops unused HashMap import. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Replaces in-method startDaprApp + run.checkRunState polling with a per-class DaprContainer in @BeforeAll and a dapr.isRunning() poll to verify shutdown. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Boots Dapr via DaprContainer with the redisConfigStore component, seeds the shared SharedTestInfra Redis via Jedis (replacing the previous docker-exec redis-cli shell-out which targeted the dapr_redis container). Adds jedis 5.1.0 as a sdk-tests test dependency. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
AbstractStateClientIT now extends BaseContainerIT instead of BaseIT. GRPCStateClientIT uses DaprContainer + redisStateStore (actor store enabled) per-class via @BeforeAll. The single MongoDB-dependent test (saveAndQueryAndDeleteState) is @disabled — out of scope for the Testcontainers migration per spec D9 / Non-Goals. QUERY_STATE_STORE constant is moved from the inherited BaseIT into AbstractStateClientIT locally since the @disabled test still references it (kept compilable for the eventual MongoDB-on-Testcontainers follow-up). Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Lazy-init openzipkin/zipkin on the shared Network with alias 'zipkin'. Used by TracingIT migrations in Phase 6 of the Testcontainers migration. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Uses startAppAndAttach to spawn MyActorService in a subprocess and wire the DaprContainer's appPort/appChannelAddress to host.testcontainers.internal. Adds a newActorClient(dapr, metadata) overload to BaseContainerIT for the second test which overrides Content-Length via actor metadata. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Lifecycle shifts from in-method startDaprApp to per-class @BeforeAll using startAppAndAttach with DemoActorService. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Per-class @BeforeAll using startAppAndAttach with MyActorService. buildManagedChannel reads gRPC port from dapr.getGrpcPort() (the DaprContainer's mapped port) instead of the global Properties.GRPC_PORT which would default to 50001 and miss the container's ephemeral port. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Per-class @BeforeAll using startAppAndAttach with MyActorService. Both proxy builders share the per-class actorClient. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
@beforeeach -> @BeforeAll per spec D9. The two mutating @test methods target different endpoints (/messages vs /persons) so per-class lifecycle is safe. Other three tests are stateless. App name is now the literal "methodinvoke-http-it" (kebab-case, replacing the prior "MethodInvokeIThttp" auto-generated from class name). Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
@beforeeach -> @BeforeAll per spec D9. DaprContainer told to speak gRPC to the app via .withAppProtocol(DaprProtocol.GRPC). createGrpcStub is now private static using APP_NAME literal. Only one test (testInvoke) mutates the message store; the two sleep tests are stateless. Per-class lifecycle is safe. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Wires Zipkin tracing via Configuration + ZipkinTracingConfigurationSettings so Dapr pushes spans to the shared Zipkin container (alias zipkin:9411). The test JVM's OpenTelemetry SDK and the Validation REST query both use the host-mapped Zipkin port. Adds zipkin-URL-aware overloads to OpenTelemetry.createOpenTelemetry and Validation.validate so the migrated IT can pass the testcontainerized host:port. The legacy single-Zipkin-URL overloads remain unchanged for back-compat. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Two previously-deleted legacy ITs had unique coverage that the migration didn't carry over. Port both into the existing migrated harnesses: - testPubSubBulkSubscribe → DaprPubSubIT Publishes NUM_MESSAGES events to the bulk-subscribe topic (topicBulkSub), retrieves the recorded responses from the subscriber via invokeMethod, and asserts a single bulk batch with NUM_MESSAGES entries all SUCCESS. The @BulkSubscribe controller route was already in SubscriberController; only the test itself was missing. - rawGrpcStubGetAndDeleteState → GRPCStateClientIT Replaces the deleted HelloWorldClientIT, exercising DaprClient.newGrpcStub(...) — the public API for getting a raw DaprGrpc.DaprBlockingStub through the SDK's managed channel. Saves a value, fetches via the raw stub, deletes via the raw stub, and re-fetches to confirm an empty response. No companion subprocess is needed (legacy used HelloWorldGrpcStateService for this). Not run locally (Docker Desktop unavailable); both files compile. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
|
@javier-aliaga / @dapr/maintainers-java-sdk / @cicoyle This PR reduce some flakiness when running integration tests. I ran 5-6 times and all of them succeeded. Could you folks please review ? Thank you. |
|
Looks like last commit is having some issues Restore parity for two legacy ITs deleted earlier Let me look into that |
- rawGrpcStubGetAndDeleteState: previously used DaprClient.saveState()
to seed the value, which JSON-serializes the string ("Hello World")
before storage. The raw gRPC stub then read back the JSON-encoded
bytes and the assertion failed (expected "Hello World", got
"\"Hello World\""). Save via the raw stub too, using a CommonProtos
StateItem with the value as raw UTF-8 bytes — the test now exercises
raw save/get/delete end-to-end.
- testPubSubBulkSubscribe: with pubsub.in-memory and synchronous
publishEvent(...).block() calls, daprd processes each publish before
the next is enqueued, so the @BulkSubscribe handler's 100ms wait
window batches each message into its own response (10 batches of 1
rather than 1 batch of 10 the legacy test asserted on). Bulk-subscribe
doesn't guarantee batch size — only that messages are delivered via
the bulk endpoint. Assert on the actual contract: total entries
across all batches == NUM_MESSAGES, all SUCCESS.
Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
When legacy ITs run via `dapr run --resources-path ./components`, daprd scans every file in that directory looking for Component, Subscription, MCPServer, and WorkflowAccessPolicy resources. secret.json isn't a Dapr resource — it's the on-disk data for secretstores.local.file — so daprd emits four warnings per legacy test invocation about "non-YAML <kind> file secret.json was detected, it will not be loaded". Move secret.json to sdk-tests/secrets/secret.json and update secretstore.yaml accordingly. Migrated SecretsClientIT already inlines its secrets into the container via withCopyToContainer, so this only affects the legacy harness path. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Jackson deserializes the controller's List<BulkSubscribeAppResponse> return value to List<LinkedHashMap> at runtime (the generic parameter erases). The implicit cast inside the enhanced-for loop blew up with ClassCastException. Take the response as List<?> and convertValue each item from Object — matches the original code's pattern that used messages.get(0) to bypass the cast. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
|
@javier-aliaga / @cicoyle / @dapr/maintainers-java-sdk This PR is ready to be reveiwed. Ran the tests 5-6 times and all the instances succeeded. So, we should definitely see a reduction in number of failures due to flakiness |
|
I think next task would be to see how we can reduce the tests runtime to lower than 20 mins |
The legacy BindingIT.httpOutputBindingError test asserted a 404 response from `https://api.github.com/unknown_path`. GitHub now returns 403 for unauthenticated requests to unknown paths, so the assertion fails. httpbin.org/status/404 is purpose-built to return whatever status code you ask for, eliminating the GitHub API-policy coupling. The component name stays as `github-http-binding-404` to avoid churning unrelated references in the test code — the historical name is harmless. Long-term, this test should be migrated to WireMock so the upstream behavior is fully under test control, but the URL swap unblocks CI now. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
This reverts commit f1efe9f. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
8202704 to
3e60255
Compare
|
@javier-aliaga / @cicoyle Just a bump on this PR. Would like to merge and get as many runs as possible to get data points |
| running.set(false); | ||
| } | ||
| // Double the backoff for the next reconnect, capped at 30s. | ||
| backoffMs = Math.min(backoffMs * 2, 30_000L); |
There was a problem hiding this comment.
Where do we reset the backoff?
| @Override | ||
| public void onError(Throwable throwable) { | ||
| listener.onError(DaprException.propagate(throwable)); | ||
| receiverStateChange.release(); |
There was a problem hiding this comment.
this still applies https://github.com/dapr/java-sdk/pull/1741/changes#r3265853071
There was a problem hiding this comment.
Repasting the comment for easy reference. Apologies for missing it
This call is a sync call, no grpc stream call, this releases the lock but is not telling the stream to be closed.
2 possibilities here;
Do we want to kill the stream because one event fails? or we just notifiy the listener and we keep going?
} catch (Exception e) {
listener.onError(DaprException.propagate(e));
// optionally ack as RETRY so the broker can redeliver
// do NOT call this.onError and do NOT release the semaphore
}
If we want to close the stream then
} catch (Exception e) {
var s = streamRef.get();
if (s != null) {
try {
s.onError(DaprException.propagate(e)); // client-side cancel of the gRPC stream
} catch (Throwable ignore) {
// already terminated
}
}
// do NOT call this.onError(...) or release the semaphore here —
// gRPC will invoke the server→client onError callback on cancellation,
// which is the path at line 126, and that one releases the semaphore.
}
I prefer the listener option, wdyt?
…ream on per-event error Addresses two review comments from Javier on PR dapr#1754: 1. Backoff was doubled up to 30s on every reconnect but never reset. Once a healthy stream disconnected after running for a while, the next reconnect (and every one thereafter) waited the capped 30s. Now the backoff resets to 1s if the just-terminated stream delivered at least one event. 2. The onNext catch-all called this.onError(...) which released the receiverStateChange semaphore and triggered a full reconnect on any per-event failure (e.g., a bad payload). It also did not actually cancel the gRPC stream. Now we just notify the listener via listener.onError(...) and keep the stream alive -- one bad event should not tear down the whole subscription. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Reverts all branch-local edits to sdk/src/main/java/io/dapr/client/ Subscription.java (commits 2dfc129, 52ba75f, 783a77d, ed8b398) so the file matches upstream/master. The backoff/error-handling changes will be revisited separately -- keeping this PR focused on the Testcontainers migration. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
…andling Restores ed8b398 (which was reverted in 731f319). Reverting to master caused failures; adding back both changes: 1. Reset reconnect backoff to 1s if the just-terminated stream had delivered at least one event (tracked per-stream via streamReceivedEvent). 2. On per-event errors in onNext, call listener.onError(...) instead of this.onError(...) so a single bad event does not tear down the whole subscription. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
New sdk/src/test/java/io/dapr/client/SubscriptionTest.java covers: - perEventExceptionKeepsStreamAlive: an exception thrown while handling an event must notify the listener without triggering a reconnect of the whole subscription. - backoffResetsAfterHealthyStreamDisconnect: reconnect backoff escalates on empty streams but resets to 1s once a stream has delivered at least one event. - closeInterruptsReconnectBackoff: close() must unblock the receiver even while it is sleeping in its reconnect backoff, so shutdown is not delayed by up to 30s. Subscription.close() now also interrupts the receiver thread so the third test passes and so real callers aren't blocked waiting for the receiver's Thread.sleep(backoffMs) to return. Signed-off-by: Siri Varma Vegiraju <siri.varma@outlook.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1754 +/- ##
============================================
+ Coverage 76.93% 77.00% +0.07%
- Complexity 2318 2322 +4
============================================
Files 245 245
Lines 7180 7194 +14
Branches 756 759 +3
============================================
+ Hits 5524 5540 +16
+ Misses 1289 1288 -1
+ Partials 367 366 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Phase 1 — Fix Flaky Integration Tests + Migrate Some ITs to Testcontainers
What this PR does
BaseIT/DaprRunharness (which requires a host-installed Dapr CLI +dapr init) onto Testcontainers viaDaprContainer, matching the spring-boot-4-sdk-tests module.Notable behavior fixes
BaseContainerIT.startAppAndAttachnow startsAppRunbefore the daprd container, with daprd's HTTP/gRPC host ports pinned (viasetPortBindings) to the sameDaprPortsvalues the app's env already received. Previously daprd would probe the app channel before the JVM had bound the host port, fetch/dapr/configagainst the SSH bridge, get nothing, report actor types[]to placement, and never re-query.calllocal/<APP_NAME>/<method>; the assertion expected the bogus<APP_NAME>-servicesuffix. Fixed in both grpc and http TracingITs.sdk/src/main/java/io/dapr/client/Subscription.java) — receiver thread now releases the state-change semaphore ononError, sleeps with a doubled-long backoff (1s → 30s cap, no overflow path), and is interrupted onclose()so shutdown isn't blocked by a backoff sleep.Files deleted in this PR — and why
pubsub/http/PubSubIT.javatestcontainers/pubsub/http/DaprPubSubIT.javaBaseITharness. Method mapping:publishPubSubNotFound→shouldReceiveInvalidArgument,testBulkPublishPubSubNotFound→shouldReceiveInvalidArgumentWithBulkPublish,testBulkPublish→shouldPublishSomePayloadTypesWithNoError,testPubSub→testPubSub,testPubSubBinary→shouldPublishBinary,testPubSubTTLMetadata→testPubSubTTLMetadata,testLongValues→testLongValues.testPubSubBulkSubscribewas missing — added on the migratedDaprPubSubITin this PR.pubsub/http/SubscriberController.javaPubSubIT. The Testcontainers IT runs the Spring Boot subscriber inside the test JVM, so a separate process file isn't needed; a differentSubscriberControlleralready lives undertestcontainers/pubsub/http/.pubsub/http/SubscriberService.javamain()entry-point that spawned the legacy subscriber. No replacement needed.pubsub/stream/PubSubStreamIT.javatestcontainers/pubsub/stream/DaprPubSubStreamIT.javatestPubSubDeadLetterTopicwas missing — added on the migratedDaprPubSubStreamITin this PR.state/HelloWorldClientIT.javastate/GRPCStateClientIT.rawGrpcStubGetAndDeleteStateDaprClient.newGrpcStub(...)(rawDaprGrpc.DaprBlockingStubsave/get/delete). Restored as a method on the migratedGRPCStateClientIT— no companion subprocess needed.state/HelloWorldGrpcStateService.javaHelloWorldClientIT.Mongo state store
AbstractStateClientIT.saveAndQueryAndDeleteState(Dapr preview Query State API, requires Mongo) was briefly@Disabledduring the migration since Redis doesn't support that API and we'd dropped the docker-compose Mongo service. Now restored:SharedTestInfra.mongo()runs a pinnedmongo:7Testcontainer, andBaseContainerIT.mongoStateStore(...)registers astate.mongodbcomponent onGRPCStateClientIT.Container image pinning
SharedTestInfranow pins all three shared containers (redis:7-alpine,mongo:7,openzipkin/zipkin:3.4) instead of letting any of them float on:latest.ITs intentionally NOT migrated (per spec Non-Goals)
BindingIT,ActorReminderFailoverIT,ActorReminderRecoveryIT,ActorTimerRecoveryIT,ActorStateIT,WaitForSidecarIT,ActorSdkResiliencyIT, plus 2 ITs indurabletask-client/. These remain on the legacyBaseIT/DaprRunharness.Checklist