Skip to content

fix: flaky FIRCLSLoggingTests.m#15608

Merged
ncooke3 merged 1 commit intomainfrom
nc/cls-log
Dec 17, 2025
Merged

fix: flaky FIRCLSLoggingTests.m#15608
ncooke3 merged 1 commit intomainfrom
nc/cls-log

Conversation

@ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Dec 16, 2025

Snippet from Nightly Report

From https://github.com/firebase/firebase-ios-sdk/actions/runs/20258318035/job/58164937354#step:7:1445:

  Test Suite 'FIRCLSLoggingTests' started at 2025-12-16 06:13:42.494.
      ✔ testEmptyKeysAndValues (0.005 seconds)
      ✔ testKeysAndValuesLog (0.006 seconds)
      ✔ testKeysAndValuesLogKeyCompaction (0.085 seconds)
      ✔ testKeysAndValuesLogMoreThanMaxKeys (0.090 seconds)
      ✔ testKeysAndValuesWithNilValue (0.005 seconds)
      ✔ testKeyValueLog (0.009 seconds)
      ✔ testKeyValueLogMoreThanMaxKeys (0.033 seconds)
      ✔ testKeyValueLogSingleKeyCompaction (0.069 seconds)
      ✔ testKeyValueWithNilKey (0.011 seconds)
      ✔ testKeyValueWithNilKeyAndValue (0.010 seconds)
  Error:     testKeyValueWithNilValue, (([[self incrementalKeyValues] count]) equal to (1)) failed: ("0") is not equal to ("1")
      ✔ testKeyValueWithNilValueCompaction (0.035 seconds)
      ✔ testLargeLogLine (0.016 seconds)
      ✔ testLoggedError (0.010 seconds)
      ✔ testLoggedErrorWithNullsInAdditionalInfo (0.005 seconds)
      ✔ testUserLog (0.016 seconds)
      ✔ testUserLogNil (0.013 seconds)
      ✔ testUserLogRotation (0.557 seconds)
      ✔ testUserLogRotationBackToBeginning (0.926 seconds)
      ✔ testWritingMaximumNumberOfLoggedErrors (0.009 seconds)
  Executed 20 tests, with 1 failure (0 unexpected) in 1.944 (1.996) seconds

Hypothesis

Queue work left over from previous test cases is affecting state in subsequent tests.

Approach

  1. Search for "dispatch_async" and inspect all call hierarchies to determine which async dispatches are reached via FIRCLSLoggingTests test suite. This should give us all the possible async culprits.
  2. For the found tests, ignore the the ones that run after the testKeyValueWithNilValue flake in the nightly test log.
  3. The remaining tests run before the flake and spawn in some way async work. In this case, the async work was always on the globally shared logging queue.
Screenshot 2025-12-16 at 10 02 40 AM

Here is a snippet of the underlying async dispatch (line 311):

dispatch_sync(FIRCLSGetLoggingQueue(), ^{
FIRCLSUserLoggingWriteKeysAndValues(sanitizedKeysAndValues, storage, counter,
containsNullValue);
});
}
static void FIRCLSUserLoggingWriteKeysAndValues(NSDictionary *keysAndValues,
FIRCLSUserLoggingKVStorage *storage,
uint32_t *counter,
BOOL containsNullValue) {
FIRCLSFile file;
if (!FIRCLSIsValidPointer(storage) || !FIRCLSIsValidPointer(counter)) {
FIRCLSSDKLogError("Bad parameters\n");
return;
}
if (!FIRCLSFileInitWithPath(&file, storage->incrementalPath, true)) {
FIRCLSSDKLogError("Unable to open k-v file\n");
return;
}
FIRCLSUserLoggingWriteKVEntriesToFile(keysAndValues, true, &file);
FIRCLSFileClose(&file);
*counter += keysAndValues.count;
if (*counter >= storage->maxIncrementalCount || containsNullValue) {
dispatch_async(FIRCLSGetLoggingQueue(), ^{
FIRCLSUserLoggingCompactKVEntries(storage);
*counter = 0;
});
}
}

Solution

Draining the logging queue in the FIRCLSLoggingTests test suite's -(void)setup method should hopefully provide a clean slate for each test.

Risks

It doesn't appear that the assertion depends on the finishing of the previous async work so the setup drain should be sufficient, but if flakes persist, we can replace/add a queue draining before the assertion.

// Flaking test...
- (void)testKeysAndValuesWithNilValue {
  FIRCLSUserLoggingRecordUserKeysAndValues(@{@"mykey" : [NSNull null]}); // <---- Spawns async work

  //// –––––––––––––––––––––––––––––––––––
  // Add queue draining here. 
  //// –––––––––––––––––––––––––––––––––––

  XCTAssertEqual([[self incrementalKeyValues] count], 1, @"");
}

#no-changelog

@ncooke3
Copy link
Member Author

ncooke3 commented Dec 16, 2025

CI results, FIRCLSLoggingTests is passing. (catalyst flakes unrelated)

@ncooke3 ncooke3 requested a review from themiswang December 16, 2025 15:39
@ncooke3 ncooke3 requested a review from andrewheard December 16, 2025 15:42
Copy link
Contributor

@themiswang themiswang left a comment

Choose a reason for hiding this comment

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

Thank you for the change!

@ncooke3 ncooke3 merged commit db15551 into main Dec 17, 2025
50 of 51 checks passed
@ncooke3 ncooke3 deleted the nc/cls-log branch December 17, 2025 14:56
JesusRojass pushed a commit to JesusRojass/firebase-ios-sdk that referenced this pull request Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants