Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Add flag for tests to avoid double-reporting check (#12569)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
uhoreg authored Jun 6, 2024
1 parent 39d0017 commit c4c1faf
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 2 deletions.
10 changes: 8 additions & 2 deletions src/DecryptionFailureTracker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down
15 changes: 15 additions & 0 deletions test/DecryptionFailureTracker-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ describe("DecryptionFailureTracker", function () {
const tracker = new DecryptionFailureTracker(
() => count++,
() => "UnknownError",
false,
);

tracker.addVisibleEvent(failedDecryptionEvent);
Expand All @@ -78,6 +79,7 @@ describe("DecryptionFailureTracker", function () {
reportedRawCode = rawCode;
},
() => "UnknownError",
false,
);

tracker.addVisibleEvent(failedDecryptionEvent);
Expand All @@ -101,6 +103,7 @@ describe("DecryptionFailureTracker", function () {
const tracker = new DecryptionFailureTracker(
() => count++,
() => "UnknownError",
false,
);

eventDecrypted(tracker, failedDecryptionEvent, Date.now());
Expand All @@ -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
Expand Down Expand Up @@ -161,6 +165,7 @@ describe("DecryptionFailureTracker", function () {
expect(true).toBe(false);
},
() => "UnknownError",
false,
);

tracker.addVisibleEvent(decryptedEvent);
Expand Down Expand Up @@ -189,6 +194,7 @@ describe("DecryptionFailureTracker", function () {
expect(true).toBe(false);
},
() => "UnknownError",
false,
);

eventDecrypted(tracker, decryptedEvent, Date.now());
Expand Down Expand Up @@ -216,6 +222,7 @@ describe("DecryptionFailureTracker", function () {
const tracker = new DecryptionFailureTracker(
() => count++,
() => "UnknownError",
false,
);

tracker.addVisibleEvent(decryptedEvent);
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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({
Expand All @@ -475,6 +485,7 @@ describe("DecryptionFailureTracker", function () {
},
// @ts-ignore access to private member
DecryptionFailureTracker.instance.errorCodeMapFn,
false,
);

const now = Date.now();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -597,6 +609,7 @@ describe("DecryptionFailureTracker", function () {
errorCount++;
},
(error: string) => error,
false,
);

// Calling .start will start some intervals. This test shouldn't run
Expand Down Expand Up @@ -638,6 +651,7 @@ describe("DecryptionFailureTracker", function () {
propertiesByErrorCode[errorCode] = properties;
},
(error: string) => error,
false,
);

// @ts-ignore access to private method
Expand Down Expand Up @@ -710,6 +724,7 @@ describe("DecryptionFailureTracker", function () {
failure = properties;
},
() => "UnknownError",
false,
);

tracker.addVisibleEvent(failedDecryptionEvent);
Expand Down

0 comments on commit c4c1faf

Please sign in to comment.