From c4c1faff979223a7daa469411b473c40a9d4dfbb Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 6 Jun 2024 09:02:34 -0400 Subject: [PATCH] Add flag for tests to avoid double-reporting check (#12569) * Add flag for tests to avoid double-reporting check Some of the tests were flaking. Our best guess is that it's failing to track some events due to false positives in the Bloom filter used to guard against double-reporting. So we add a flag to disable using that in tests (except for tests that test that functionality). * invert logic to avoid double negative --- src/DecryptionFailureTracker.ts | 10 ++++++++-- test/DecryptionFailureTracker-test.ts | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/DecryptionFailureTracker.ts b/src/DecryptionFailureTracker.ts index cc336cc5ce..548555cd1d 100644 --- a/src/DecryptionFailureTracker.ts +++ b/src/DecryptionFailureTracker.ts @@ -158,10 +158,16 @@ export class DecryptionFailureTracker { * * @param {function} errorCodeMapFn The function used to map decryption failure reason codes to the * `trackedErrorCode`. + * + * @param {boolean} checkReportedEvents Check if we have already reported an event. + * Defaults to `true`. This is only used for tests, to avoid possible false positives from + * the Bloom filter. This should be set to `false` for all tests except for those + * that specifically test the `reportedEvents` functionality. */ private constructor( private readonly fn: TrackingFn, private readonly errorCodeMapFn: ErrCodeMapFn, + private readonly checkReportedEvents: boolean = true, ) { if (!fn || typeof fn !== "function") { throw new Error("DecryptionFailureTracker requires tracking function"); @@ -214,7 +220,7 @@ export class DecryptionFailureTracker { const eventId = e.getId()!; // if it's already reported, we don't need to do anything - if (this.reportedEvents.has(eventId)) { + if (this.reportedEvents.has(eventId) && this.checkReportedEvents) { return; } @@ -240,7 +246,7 @@ export class DecryptionFailureTracker { const eventId = e.getId()!; // if it's already reported, we don't need to do anything - if (this.reportedEvents.has(eventId)) { + if (this.reportedEvents.has(eventId) && this.checkReportedEvents) { return; } diff --git a/test/DecryptionFailureTracker-test.ts b/test/DecryptionFailureTracker-test.ts index 06fe22ce54..ca8c4b3660 100644 --- a/test/DecryptionFailureTracker-test.ts +++ b/test/DecryptionFailureTracker-test.ts @@ -52,6 +52,7 @@ describe("DecryptionFailureTracker", function () { const tracker = new DecryptionFailureTracker( () => count++, () => "UnknownError", + false, ); tracker.addVisibleEvent(failedDecryptionEvent); @@ -78,6 +79,7 @@ describe("DecryptionFailureTracker", function () { reportedRawCode = rawCode; }, () => "UnknownError", + false, ); tracker.addVisibleEvent(failedDecryptionEvent); @@ -101,6 +103,7 @@ describe("DecryptionFailureTracker", function () { const tracker = new DecryptionFailureTracker( () => count++, () => "UnknownError", + false, ); eventDecrypted(tracker, failedDecryptionEvent, Date.now()); @@ -121,6 +124,7 @@ describe("DecryptionFailureTracker", function () { propertiesByErrorCode[errorCode] = properties; }, (error: string) => error, + false, ); // use three different errors so that we can distinguish the reports @@ -161,6 +165,7 @@ describe("DecryptionFailureTracker", function () { expect(true).toBe(false); }, () => "UnknownError", + false, ); tracker.addVisibleEvent(decryptedEvent); @@ -189,6 +194,7 @@ describe("DecryptionFailureTracker", function () { expect(true).toBe(false); }, () => "UnknownError", + false, ); eventDecrypted(tracker, decryptedEvent, Date.now()); @@ -216,6 +222,7 @@ describe("DecryptionFailureTracker", function () { const tracker = new DecryptionFailureTracker( () => count++, () => "UnknownError", + false, ); tracker.addVisibleEvent(decryptedEvent); @@ -379,6 +386,7 @@ describe("DecryptionFailureTracker", function () { (errorCode: string) => (counts[errorCode] = (counts[errorCode] || 0) + 1), (error: DecryptionFailureCode) => error === DecryptionFailureCode.UNKNOWN_ERROR ? "UnknownError" : "OlmKeysNotSentError", + false, ); const decryptedEvent1 = await createFailedDecryptionEvent({ @@ -416,6 +424,7 @@ describe("DecryptionFailureTracker", function () { const tracker = new DecryptionFailureTracker( (errorCode: string) => (counts[errorCode] = (counts[errorCode] || 0) + 1), (_errorCode: string) => "OlmUnspecifiedError", + false, ); const decryptedEvent1 = await createFailedDecryptionEvent({ @@ -450,6 +459,7 @@ describe("DecryptionFailureTracker", function () { const tracker = new DecryptionFailureTracker( (errorCode: string) => (counts[errorCode] = (counts[errorCode] || 0) + 1), (errorCode: string) => Array.from(errorCode).reverse().join(""), + false, ); const decryptedEvent = await createFailedDecryptionEvent({ @@ -475,6 +485,7 @@ describe("DecryptionFailureTracker", function () { }, // @ts-ignore access to private member DecryptionFailureTracker.instance.errorCodeMapFn, + false, ); const now = Date.now(); @@ -543,6 +554,7 @@ describe("DecryptionFailureTracker", function () { propertiesByErrorCode[errorCode] = properties; }, (error: string) => error, + false, ); // use three different errors so that we can distinguish the reports @@ -597,6 +609,7 @@ describe("DecryptionFailureTracker", function () { errorCount++; }, (error: string) => error, + false, ); // Calling .start will start some intervals. This test shouldn't run @@ -638,6 +651,7 @@ describe("DecryptionFailureTracker", function () { propertiesByErrorCode[errorCode] = properties; }, (error: string) => error, + false, ); // @ts-ignore access to private method @@ -710,6 +724,7 @@ describe("DecryptionFailureTracker", function () { failure = properties; }, () => "UnknownError", + false, ); tracker.addVisibleEvent(failedDecryptionEvent);