Skip to content

chore: add cloudwatch client integration tests - #3389

Open
thisisabhash wants to merge 3 commits into
feat/v3-cloudwatch-clientfrom
cloudwatch-client-3
Open

chore: add cloudwatch client integration tests#3389
thisisabhash wants to merge 3 commits into
feat/v3-cloudwatch-clientfrom
cloudwatch-client-3

Conversation

@thisisabhash

Copy link
Copy Markdown
Member
  • PR title and description conform to Pull Request guidelines.

Issue #, if available:

N/A

Description of changes:

Adds connected integration tests for the standalone v3 CloudWatch client (AmplifyCloudWatchClient), structured after the :aws-kinesis client's instrumentation tests. (Stacked on the client implementation — this PR is only the tests.)

  • AmplifyCloudWatchClientInstrumentationTest (:aws-cloudwatch/src/androidTest, extends DeviceFarmTestBase) exercises the real client against a provisioned CloudWatch Logs backend:

    • testGetEscapeHatchgetCloudWatchLogsClient() returns the underlying AWS SDK client.
    • testFlushLogWithMessages — emitted messages are flushed and land in the log group.
    • testFlushLogWithVerboseMessageAfterEnabling — a verbose message emitted while enabled is delivered.
    • testFlushLogWithVerboseMessageAfterDisabling — a verbose message emitted while disabled is dropped (not delivered).

    Delivery is confirmed by reading the log group back via filterLogEvents, polling with a re-flush between attempts since CloudWatch ingestion is eventually consistent.

  • Credentials come from the provisioned backend's Cognito identity pool (guest access — no sign-in). Backend config is read from two gitignored, CI-injected resources under src/androidTest/res/raw/: amplify_outputs.json (Auth) and amplifyconfiguration_logging.json (region, log group, etc.). The CI test-config step must supply both, and the target log group must be provisioned.

  • Adds :core, :aws-core, and :aws-auth-cognito as androidTestImplementation dependencies for the Amplify Auth / Cognito credential setup.

How did you test these changes?

  • Ran ./gradlew :aws-cloudwatch:connectedDebugAndroidTest on an emulator against the provisioned backend — 10/10 connected tests pass (these 4 client tests plus the module's 6 existing DB instrumentation tests).

Documentation update required?

  • No
  • Yes (Please include a PR link for the documentation update)

General Checklist

  • Added Unit Tests
  • Added Integration Tests
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
  • Ensure commit message has the appropriate scope (e.g fix(storage): message, feat(auth): message, chore(all): message)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@thisisabhash
thisisabhash requested a review from a team as a code owner August 20, 2026 18:16
Base automatically changed from cloudwatch-client-2 to feat/v3-cloudwatch-client August 21, 2026 16:03
@mattcreaser
mattcreaser requested a review from a team as a code owner August 21, 2026 16:03
Comment on lines +189 to +198
private suspend fun awaitEvents(message: String, expectedCount: Int): List<String> {
var events = emptyList<String>()
repeat(MAX_FLUSH_ATTEMPTS) { attempt ->
client.flushLogs()
delay(INGEST_WAIT_MS)
events = filterEvents(message, windowMinutes = attempt + 2)
if (events.size >= expectedCount) return events
}
return events
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔴 Critical — the discarded flushLogs() result makes the negative test vacuous.

flushLogs() returns FlushResult = Result<FlushData, AmplifyCloudWatchException>; it catches everything and reports failure in the return value rather than throwing. Discarding it means a flush that fails on credentials, a missing log group, or throttling is indistinguishable from a successful one.

That is merely a bad error message for the positive tests, but it is fatal for testFlushLogWithVerboseMessageAfterDisabling: with expectedCount = 0 the shouldBeEmpty() assertion holds when every single flush failed. The test would pass against a completely broken client, so it certifies nothing.

Suggested change
private suspend fun awaitEvents(message: String, expectedCount: Int): List<String> {
var events = emptyList<String>()
repeat(MAX_FLUSH_ATTEMPTS) { attempt ->
client.flushLogs()
delay(INGEST_WAIT_MS)
events = filterEvents(message, windowMinutes = attempt + 2)
if (events.size >= expectedCount) return events
}
return events
}
private suspend fun awaitEvents(message: String, expectedCount: Int): List<String> {
var events = emptyList<String>()
repeat(MAX_FLUSH_ATTEMPTS) { attempt ->
client.flushLogs().shouldBeSuccess()
delay(INGEST_WAIT_MS)
events = filterEvents(message, windowMinutes = attempt + 2)
if (events.size >= expectedCount) return events
}
return events
}

shouldBeSuccess is the existing :testutils assertion for Result. Even with that in place the disabled-path test has no positive control — consider emitting one Error-level message alongside the dropped Verbose one and asserting the error arrives while the verbose one does not, so delivery failure can no longer masquerade as correct filtering.

Comment on lines +164 to +165
events shouldHaveSize 1
events.first().lowercase() shouldContain "verbose"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Warning — this assertion cannot fail.

The intent is to check the client's "${level.name.lowercase()}/${name}: ..." prefix, but message is "this is a verbose message after enabling …" (line 156) — it already contains the word "verbose". The substring check is satisfied by the body regardless of the level actually recorded, so an Error-level prefix would pass too.

Anchor it to the prefix instead:

Suggested change
events shouldHaveSize 1
events.first().lowercase() shouldContain "verbose"
events shouldHaveSize 1
events.first() shouldStartWith "verbose/$namespace: "

(io.kotest.matchers.string.shouldStartWith; this subsumes the namespace assertion on line 167.) Renaming the message payload so it does not contain the level name would also help keep the two independent.

Comment on lines +116 to +120
options = AmplifyCloudWatchClientOptions {
logGroupName = testLogGroupName
localStoreMaxSizeInMB = testLocalStoreMaxSizeInMB
flushStrategy = FlushStrategy.Interval(testFlushIntervalInSeconds.seconds)
loggingConstraints = LoggingConstraints(defaultLogLevel = testDefaultLogLevel)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Warning — the pass/fail threshold is read from external config while the expected counts are hard-coded.

defaultLogLevel comes from amplifyconfiguration_logging.json, but testFlushLogWithMessages asserts exactly 4 events for Error/Debug/Warn/Info and testFlushLogWithVerboseMessageAfterEnabling asserts 1 for Verbose. CloudWatchLoggingFilter drops anything below the threshold, so an edit to the S3-hosted config silently breaks the suite: Error yields 1 of 4, and anything above Verbose makes the verbose test fail outright. The tests only pass today because the config happens to say VERBOSE.

Pin what the assertions depend on:

Suggested change
options = AmplifyCloudWatchClientOptions {
logGroupName = testLogGroupName
localStoreMaxSizeInMB = testLocalStoreMaxSizeInMB
flushStrategy = FlushStrategy.Interval(testFlushIntervalInSeconds.seconds)
loggingConstraints = LoggingConstraints(defaultLogLevel = testDefaultLogLevel)
options = AmplifyCloudWatchClientOptions {
logGroupName = testLogGroupName
localStoreMaxSizeInMB = testLocalStoreMaxSizeInMB
flushStrategy = FlushStrategy.None
loggingConstraints = LoggingConstraints(defaultLogLevel = LogLevel.Verbose)
}

This also drops testDefaultLogLevel and its logLevelOf parsing helper (lines 78, 99, 102-104) — the config's loggingConstraints block no longer needs reading. If you would rather keep exercising the config value, derive the expectations from it instead of hard-coding 4 and 1.

options = AmplifyCloudWatchClientOptions {
logGroupName = testLogGroupName
localStoreMaxSizeInMB = testLocalStoreMaxSizeInMB
flushStrategy = FlushStrategy.Interval(testFlushIntervalInSeconds.seconds)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Warning — interval auto-flush races the explicit flushes.

FlushStrategy.Interval starts a WorkManager sync from init, so a background flush can fire between an emit() and the filterEvents() poll. The :aws-kinesis tests deliberately avoid this — every builder there passes FlushStrategy.None with the comment "Don't auto-flush as it may impact other tests" — except in the two tests specifically covering auto-flush.

Since awaitEvents drives flushing explicitly, the interval buys nothing here and adds nondeterminism (it is also why clearPackageData=false matters more than it should). Use FlushStrategy.None, which makes testFlushIntervalInSeconds and its config read (lines 77, 98) dead too. If interval flushing is worth covering, it deserves its own test that asserts delivery without calling flushLogs().

Comment on lines +109 to +123
@Before
fun setUp() {
val context = ApplicationProvider.getApplicationContext<Context>()
client = AmplifyCloudWatchClient(
context = context,
region = testRegion,
credentialsProvider = credentialsProvider,
options = AmplifyCloudWatchClientOptions {
logGroupName = testLogGroupName
localStoreMaxSizeInMB = testLocalStoreMaxSizeInMB
flushStrategy = FlushStrategy.Interval(testFlushIntervalInSeconds.seconds)
loggingConstraints = LoggingConstraints(defaultLogLevel = testDefaultLogLevel)
}
)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Warning — no @After, so every test leaks a live client.

Each test method builds a new AmplifyCloudWatchClient in @Before and nothing ever shuts it down. The client owns a CoroutineScope(SupervisorJob() + Dispatchers.IO), a CloudWatchLogsClient, a SQLCipher store, and (with FlushStrategy.Interval) scheduled WorkManager sync — so by the last test four clients are concurrently flushing the same log group and the same shared on-device DB, since databaseName is keyed on logGroupName alone.

AmplifyCloudWatchClient exposes no close(), so disable() (which calls logManager.stopSync()) is the best available teardown. :aws-kinesis's base class does exactly this:

Suggested change
@Before
fun setUp() {
val context = ApplicationProvider.getApplicationContext<Context>()
client = AmplifyCloudWatchClient(
context = context,
region = testRegion,
credentialsProvider = credentialsProvider,
options = AmplifyCloudWatchClientOptions {
logGroupName = testLogGroupName
localStoreMaxSizeInMB = testLocalStoreMaxSizeInMB
flushStrategy = FlushStrategy.Interval(testFlushIntervalInSeconds.seconds)
loggingConstraints = LoggingConstraints(defaultLogLevel = testDefaultLogLevel)
}
)
}
@Before
fun setUp() {
val context = ApplicationProvider.getApplicationContext<Context>()
client = AmplifyCloudWatchClient(
context = context,
region = testRegion,
credentialsProvider = credentialsProvider,
options = AmplifyCloudWatchClientOptions {
logGroupName = testLogGroupName
localStoreMaxSizeInMB = testLocalStoreMaxSizeInMB
flushStrategy = FlushStrategy.Interval(testFlushIntervalInSeconds.seconds)
loggingConstraints = LoggingConstraints(defaultLogLevel = testDefaultLogLevel)
}
)
}
@After
fun tearDown() {
client.disable()
}

That the client has no way to release its scope may itself be worth raising against the client PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The client should have a close function that cancels the scope

Comment on lines +143 to +145
val events = awaitEvents(message, expectedCount = 4)

events shouldHaveSize 4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 Suggestion — >= in the poll loop versus an exact-size assertion.

awaitEvents returns as soon as events.size >= expectedCount, then this asserts exactly 4. All four messages share one message string and filterPattern matches on it, so any at-least-once redelivery (a flush retry, or leftovers from an earlier run now that clearPackageData=false preserves the DB) returns 5 and fails a test that arguably should tolerate it.

Either assert events.size shouldBeGreaterThanOrEqual 4, or give each of the four emissions a distinct message and assert one event per level — the latter is stronger, since the current form cannot tell four delivered messages from the same message delivered four times.

Comment thread aws-cloudwatch/build.gradle.kts Outdated
Comment on lines +26 to +31
// Persist app data across connected test methods so the client's integration tests keep a stable device
// id and write to a single CloudWatch stream (one per test otherwise). Overrides the shared convention's
// clearPackageData=true; the module's own DB instrumentation test self-cleans in @After.
defaultConfig {
testInstrumentationRunnerArguments["clearPackageData"] = "false"
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 Suggestion — is the module-wide clearPackageData override actually needed?

The stated reason is keeping a stable device id so tests share one log stream, but the assertions never look at a stream: filterEvents calls filterLogEvents scoped to logGroupName, which spans every stream in the group. So stream fragmentation would not fail these tests, and turning this off weakens isolation for the whole module — including CloudWatchDatabaseInstrumentationTest — and lets a failed run's unflushed events persist into the next one.

If something does depend on it, please spell that out in the comment; if not, dropping the block keeps the shared convention's clearPackageData=true. Worth confirming on-device either way, since I could not run the connected suite.

}
credentialsProvider = CognitoCredentialsProvider().toAwsCredentialsProvider()

val json = context.resources.openRawResource(R.raw.amplifyconfiguration_logging)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use Resources.getRawResourceId to reference the file instead of directly using the R class, as this won't compile for anyone who doesn't have a backend available.

Comment on lines +85 to +89
if (!configured) {
Amplify.Auth.addPlugin(AWSCognitoAuthPlugin())
Amplify.configure(AmplifyOutputs(R.raw.amplify_outputs), context)
configured = true
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need to guard against reconfiguration here since this is run in @BeforeClass, so the test orchestrator will only invoke this once per process.

Comment on lines +76 to +77
private var testLocalStoreMaxSizeInMB: Int = 1
private var testFlushIntervalInSeconds: Long = 60

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These can be lateinit too

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.

2 participants