From 14976d948fd4642a0baca5fc7b2896c3bff6b163 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 20 Jun 2024 21:08:47 -0700 Subject: [PATCH] Fix #5430: Fix flakes in NetworkLoggingInterceptorTest (#5436) ## Explanation Fixes #5430 Fix flaky tests in ``NetworkLoggingInterceptorTest`` and (theoretically) in ``ConsoleLoggerTest``. See #5430 for investigation methodology and findings. Ultimately, the problem was that tests were trying to observed ``SharedFlow`` results after changes to that flow may have been emitted (which means there's no guarantees by the flow that the value is present upon trying to consume it, hence the flake in the job consuming the flow not completing). Note that ``ConsoleLoggerTest``'s fix is only theoretical since, having only one test, it doesn't seem to ever produce the correct situations that would cause the flake. Originally, ``NetworkLoggingInterceptorTest`` would flake with almost a 20% failure rate. I've verified that the test passes without failures over 1000x runs: ``` > bazel test --runs_per_test=1000 //data:src/test/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptorTest Target //data:src/test/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptorTest up-to-date: bazel-bin/data/src/test/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptorTest.jar bazel-bin/data/src/test/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptorTest INFO: Elapsed time: 833.435s, Critical Path: 103.87s INFO: 1001 processes: 1 internal, 1000 linux-sandbox. INFO: Build completed successfully, 1001 total actions //data:src/test/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptorTest PASSED in 103.3s Stats over 1000 runs: max = 103.3s, min = 38.1s, avg = 77.7s, dev = 12.4s Executed 1 out of 1 test: 1 test passes. ``` Similarly, I've also checked the change in ``ConsoleLoggerTest`` just to ensure nothing's changed: ``` > bazel test --runs_per_test=1000 //utility/src/test/java/org/oppia/android/util/logging:ConsoleLoggerTest Target //utility/src/test/java/org/oppia/android/util/logging:ConsoleLoggerTest up-to-date: bazel-bin/utility/src/test/java/org/oppia/android/util/logging/ConsoleLoggerTest.jar bazel-bin/utility/src/test/java/org/oppia/android/util/logging/ConsoleLoggerTest INFO: Elapsed time: 767.614s, Critical Path: 105.49s INFO: 1001 processes: 1 internal, 1000 linux-sandbox. INFO: Build completed successfully, 1001 total actions //utility/src/test/java/org/oppia/android/util/logging:ConsoleLoggerTest PASSED in 104.6s Stats over 1000 runs: max = 104.6s, min = 31.3s, avg = 71.6s, dev = 8.8s Executed 1 out of 1 test: 1 test passes. ``` ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only This PR only affects the behavior of two test suites; no end user UX or UIs are affected by these changes. --- .../android/data/backends/gae/NetworkLoggingInterceptorTest.kt | 3 +++ .../java/org/oppia/android/util/logging/ConsoleLoggerTest.kt | 2 ++ 2 files changed, 5 insertions(+) diff --git a/data/src/test/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptorTest.kt b/data/src/test/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptorTest.kt index e62b1bcc384..6b82cb460fb 100644 --- a/data/src/test/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptorTest.kt +++ b/data/src/test/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptorTest.kt @@ -93,6 +93,7 @@ class NetworkLoggingInterceptorTest { val firstRequestsDeferred = CoroutineScope(backgroundTestDispatcher).async { networkLoggingInterceptor.logNetworkCallFlow.take(1).toList() } + testCoroutineDispatchers.advanceUntilIdle() // Ensure the flow is subscribed before emit(). client.newCall(request).execute() testCoroutineDispatchers.advanceUntilIdle() @@ -116,6 +117,7 @@ class NetworkLoggingInterceptorTest { networkLoggingInterceptor.logFailedNetworkCallFlow.take(1).toList() } mockWebServer.enqueue(mockResponse) + testCoroutineDispatchers.advanceUntilIdle() // Ensure the flow is subscribed before emit(). client.newCall(request).execute() testCoroutineDispatchers.advanceUntilIdle() @@ -141,6 +143,7 @@ class NetworkLoggingInterceptorTest { val firstFailingRequestsDeferred = CoroutineScope(backgroundTestDispatcher).async { networkLoggingInterceptor.logFailedNetworkCallFlow.take(1).toList() } + testCoroutineDispatchers.advanceUntilIdle() // Ensure the flow is subscribed before emit(). try { client.newCall(request).execute() } catch (e: ConnectException) { diff --git a/utility/src/test/java/org/oppia/android/util/logging/ConsoleLoggerTest.kt b/utility/src/test/java/org/oppia/android/util/logging/ConsoleLoggerTest.kt index 65e37d6bdcc..6a314de75fe 100644 --- a/utility/src/test/java/org/oppia/android/util/logging/ConsoleLoggerTest.kt +++ b/utility/src/test/java/org/oppia/android/util/logging/ConsoleLoggerTest.kt @@ -65,6 +65,8 @@ class ConsoleLoggerTest { val firstErrorContextsDeferred = CoroutineScope(backgroundTestDispatcher).async { consoleLogger.logErrorMessagesFlow.take(1).toList() } + + testCoroutineDispatchers.advanceUntilIdle() // Ensure the flow is subscribed before emit(). consoleLogger.e(testTag, testMessage) testCoroutineDispatchers.advanceUntilIdle()