Skip to content

Commit

Permalink
Fix #5430: Fix flakes in NetworkLoggingInterceptorTest (#5436)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
BenHenning authored Jun 21, 2024
1 parent 6a5f55a commit 14976d9
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand All @@ -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()

Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down

0 comments on commit 14976d9

Please sign in to comment.