From acd5dde6d838a4de8fef90eaf2a7065e228559db Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 26 Nov 2025 15:31:47 +0100 Subject: [PATCH 1/9] test: use `setSystemTime` for better test stability --- src/reactions/RaisedHandIndicator.test.tsx | 9 ++++++++- .../__snapshots__/RaisedHandIndicator.test.tsx.snap | 6 +++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/reactions/RaisedHandIndicator.test.tsx b/src/reactions/RaisedHandIndicator.test.tsx index fedd8ec2..62e3ffb5 100644 --- a/src/reactions/RaisedHandIndicator.test.tsx +++ b/src/reactions/RaisedHandIndicator.test.tsx @@ -5,7 +5,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { describe, expect, test } from "vitest"; +import { beforeEach, describe, expect, test, vi } from "vitest"; import { render, configure } from "@testing-library/react"; import { RaisedHandIndicator } from "./RaisedHandIndicator"; @@ -15,6 +15,13 @@ configure({ }); describe("RaisedHandIndicator", () => { + const fixedTime = new Date("2025-01-01T12:00:00.000Z"); + + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(fixedTime); + }); + test("renders nothing when no hand has been raised", () => { const { container } = render(); expect(container.firstChild).toBeNull(); diff --git a/src/reactions/__snapshots__/RaisedHandIndicator.test.tsx.snap b/src/reactions/__snapshots__/RaisedHandIndicator.test.tsx.snap index ab6fafa3..43c3f928 100644 --- a/src/reactions/__snapshots__/RaisedHandIndicator.test.tsx.snap +++ b/src/reactions/__snapshots__/RaisedHandIndicator.test.tsx.snap @@ -15,7 +15,7 @@ exports[`RaisedHandIndicator > renders a smaller indicator when miniature is spe

- 00:01 + 00:00

`; @@ -35,7 +35,7 @@ exports[`RaisedHandIndicator > renders an indicator when a hand has been raised

- 00:01 + 00:00

`; @@ -55,7 +55,7 @@ exports[`RaisedHandIndicator > renders an indicator when a hand has been raised

- 01:01 + 01:00

`; From 2b3e2f61884ddb40335843f63adaf6548f815fd1 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 26 Nov 2025 19:13:07 +0100 Subject: [PATCH 2/9] test: Enable quality gate and touch a file --- codecov.yaml | 3 +- src/state/ObservableScope.test.ts | 51 ++++++++++++++++++++++++++++++- src/state/ObservableScope.ts | 5 +++ 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/codecov.yaml b/codecov.yaml index e1289344..f08dc9b2 100644 --- a/codecov.yaml +++ b/codecov.yaml @@ -13,7 +13,6 @@ coverage: informational: true patch: default: - # Encourage (but don't enforce) 80% coverage on all lines that a PR + # Enforce 80% coverage on all lines that a PR # touches target: 80% - informational: true diff --git a/src/state/ObservableScope.test.ts b/src/state/ObservableScope.test.ts index 99f2b424..8513b54b 100644 --- a/src/state/ObservableScope.test.ts +++ b/src/state/ObservableScope.test.ts @@ -5,7 +5,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { BehaviorSubject, combineLatest, Subject } from "rxjs"; import { logger } from "matrix-js-sdk/lib/logger"; @@ -102,3 +102,52 @@ describe("Epoch", () => { s$.complete(); }); }); + +describe("Reconcile", () => { + it("should wait for cleanup to complete before processing next Value", async () => { + vi.useFakeTimers(); + + const scope = new ObservableScope(); + const cleanUp1 = Promise.withResolvers(); + const cleanUp2 = vi.fn(); + + const behavior$ = new BehaviorSubject(1); + let lastProcessed = 0; + + scope.reconcile( + behavior$, + async (value: number): Promise<(() => Promise) | void> => { + lastProcessed = value; + if (value === 1) { + return Promise.resolve(async (): Promise => cleanUp1.promise); + } else if (value === 2) { + return Promise.resolve(async (): Promise => { + cleanUp2(); + return Promise.resolve(undefined); + }); + } + return Promise.resolve(); + }, + ); + + // behavior$.next(1); + await vi.advanceTimersByTimeAsync(200); + behavior$.next(2); + await vi.advanceTimersByTimeAsync(300); + + await vi.runAllTimersAsync(); + + // Should not have processed 2 yet because cleanup of 1 is pending + expect(lastProcessed).toBe(1); + cleanUp1.resolve(); + // await flushPromises(); + + await vi.runAllTimersAsync(); + // Now 2 should be processed + expect(lastProcessed).toBe(2); + + scope.end(); + await vi.runAllTimersAsync(); + expect(cleanUp2).toHaveBeenCalled(); + }); +}); diff --git a/src/state/ObservableScope.ts b/src/state/ObservableScope.ts index 27f501c7..87a95ba5 100644 --- a/src/state/ObservableScope.ts +++ b/src/state/ObservableScope.ts @@ -117,6 +117,10 @@ export class ObservableScope { * values may be skipped. * * Basically, this is like React's useEffect but async and for Behaviors. + * + * @arg value$ - The Behavior to track. + * @arg callback - Called whenever the value must be handled. May return a clean-up function + * */ public reconcile( value$: Behavior, @@ -221,6 +225,7 @@ export class Epoch { this.value = value; this.epoch = epoch ?? 0; } + /** * Maps the value inside the epoch to a new value while keeping the epoch number. * # usage From c078dd87fa932d0e36ee9a4f21a990a60cd44e95 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 26 Nov 2025 19:28:44 +0100 Subject: [PATCH 3/9] add untested function --- src/state/ObservableScope.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/state/ObservableScope.ts b/src/state/ObservableScope.ts index 87a95ba5..5a8e1525 100644 --- a/src/state/ObservableScope.ts +++ b/src/state/ObservableScope.ts @@ -18,7 +18,9 @@ import { share, take, takeUntil, + tap, } from "rxjs"; +import { logger } from "matrix-js-sdk/lib/logger"; import { type Behavior } from "./Behavior"; @@ -156,6 +158,17 @@ export class ObservableScope { }); } + public unTestedMethod$(input$: Behavior): Observable { + // This method is intentionally left untested. + return input$.pipe( + map((value) => value), + distinctUntilChanged(), + tap((value) => { + logger.log(`Value changed: ${value}`); + }), + ); + } + /** * Splits a Behavior of objects with static properties into an object with * Behavior properties. From 794585514a5faa2c02238b133abc443ed0a233fd Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 27 Nov 2025 10:04:09 +0100 Subject: [PATCH 4/9] Revert "add untested function" This reverts commit c078dd87fa932d0e36ee9a4f21a990a60cd44e95. --- src/state/ObservableScope.ts | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/state/ObservableScope.ts b/src/state/ObservableScope.ts index 5a8e1525..87a95ba5 100644 --- a/src/state/ObservableScope.ts +++ b/src/state/ObservableScope.ts @@ -18,9 +18,7 @@ import { share, take, takeUntil, - tap, } from "rxjs"; -import { logger } from "matrix-js-sdk/lib/logger"; import { type Behavior } from "./Behavior"; @@ -158,17 +156,6 @@ export class ObservableScope { }); } - public unTestedMethod$(input$: Behavior): Observable { - // This method is intentionally left untested. - return input$.pipe( - map((value) => value), - distinctUntilChanged(), - tap((value) => { - logger.log(`Value changed: ${value}`); - }), - ); - } - /** * Splits a Behavior of objects with static properties into an object with * Behavior properties. From 4c2c7d51cfc7c8cc747f353df34d44f3d9bfc834 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 27 Nov 2025 16:38:51 +0100 Subject: [PATCH 5/9] revert test changes on ObservableScope --- src/state/ObservableScope.test.ts | 51 +------------------------------ src/state/ObservableScope.ts | 5 --- 2 files changed, 1 insertion(+), 55 deletions(-) diff --git a/src/state/ObservableScope.test.ts b/src/state/ObservableScope.test.ts index 8513b54b..99f2b424 100644 --- a/src/state/ObservableScope.test.ts +++ b/src/state/ObservableScope.test.ts @@ -5,7 +5,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { describe, expect, it, vi } from "vitest"; +import { describe, expect, it } from "vitest"; import { BehaviorSubject, combineLatest, Subject } from "rxjs"; import { logger } from "matrix-js-sdk/lib/logger"; @@ -102,52 +102,3 @@ describe("Epoch", () => { s$.complete(); }); }); - -describe("Reconcile", () => { - it("should wait for cleanup to complete before processing next Value", async () => { - vi.useFakeTimers(); - - const scope = new ObservableScope(); - const cleanUp1 = Promise.withResolvers(); - const cleanUp2 = vi.fn(); - - const behavior$ = new BehaviorSubject(1); - let lastProcessed = 0; - - scope.reconcile( - behavior$, - async (value: number): Promise<(() => Promise) | void> => { - lastProcessed = value; - if (value === 1) { - return Promise.resolve(async (): Promise => cleanUp1.promise); - } else if (value === 2) { - return Promise.resolve(async (): Promise => { - cleanUp2(); - return Promise.resolve(undefined); - }); - } - return Promise.resolve(); - }, - ); - - // behavior$.next(1); - await vi.advanceTimersByTimeAsync(200); - behavior$.next(2); - await vi.advanceTimersByTimeAsync(300); - - await vi.runAllTimersAsync(); - - // Should not have processed 2 yet because cleanup of 1 is pending - expect(lastProcessed).toBe(1); - cleanUp1.resolve(); - // await flushPromises(); - - await vi.runAllTimersAsync(); - // Now 2 should be processed - expect(lastProcessed).toBe(2); - - scope.end(); - await vi.runAllTimersAsync(); - expect(cleanUp2).toHaveBeenCalled(); - }); -}); diff --git a/src/state/ObservableScope.ts b/src/state/ObservableScope.ts index 87a95ba5..27f501c7 100644 --- a/src/state/ObservableScope.ts +++ b/src/state/ObservableScope.ts @@ -117,10 +117,6 @@ export class ObservableScope { * values may be skipped. * * Basically, this is like React's useEffect but async and for Behaviors. - * - * @arg value$ - The Behavior to track. - * @arg callback - Called whenever the value must be handled. May return a clean-up function - * */ public reconcile( value$: Behavior, @@ -225,7 +221,6 @@ export class Epoch { this.value = value; this.epoch = epoch ?? 0; } - /** * Maps the value inside the epoch to a new value while keeping the epoch number. * # usage From e5509ba49b6f72f0a9760f8c469a3fa2ccf8610d Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 27 Nov 2025 16:39:45 +0100 Subject: [PATCH 6/9] vitest: Add threshold on vitest to check locally --- vitest.config.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/vitest.config.ts b/vitest.config.ts index 1c6f746b..4629515e 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -25,6 +25,12 @@ export default defineConfig((configEnv) => "src/utils/test-fixtures.ts", "playwright/**", ], + thresholds: { + lines: 80, + functions: 80, + branches: 80, + statements: 80, + }, }, }, }), From c00e332dd8237b256e96d499efbab20184602e29 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 27 Nov 2025 16:43:12 +0100 Subject: [PATCH 7/9] Revert "vitest: Add threshold on vitest to check locally" This reverts commit e5509ba49b6f72f0a9760f8c469a3fa2ccf8610d. --- vitest.config.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/vitest.config.ts b/vitest.config.ts index 4629515e..1c6f746b 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -25,12 +25,6 @@ export default defineConfig((configEnv) => "src/utils/test-fixtures.ts", "playwright/**", ], - thresholds: { - lines: 80, - functions: 80, - branches: 80, - statements: 80, - }, }, }, }), From cdd391b3a27a5086d299d7dd397f38f9d31c074f Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 28 Nov 2025 09:12:05 +0100 Subject: [PATCH 8/9] test: Add simple tests for scope.reconcile --- src/state/ObservableScope.test.ts | 92 ++++++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 1 deletion(-) diff --git a/src/state/ObservableScope.test.ts b/src/state/ObservableScope.test.ts index 99f2b424..a0c71268 100644 --- a/src/state/ObservableScope.test.ts +++ b/src/state/ObservableScope.test.ts @@ -5,9 +5,10 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { BehaviorSubject, combineLatest, Subject } from "rxjs"; import { logger } from "matrix-js-sdk/lib/logger"; +import { sleep } from "matrix-js-sdk/lib/utils"; import { Epoch, @@ -102,3 +103,92 @@ describe("Epoch", () => { s$.complete(); }); }); + +describe("Reconcile", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("should wait clean up before processing next", async () => { + vi.useFakeTimers(); + const scope = new ObservableScope(); + const behavior$ = new BehaviorSubject(0); + + const setup = vi.fn().mockImplementation(async () => await sleep(100)); + const cleanup = vi + .fn() + .mockImplementation(async (n: number) => await sleep(100)); + scope.reconcile(behavior$, async (value) => { + await setup(); + return async (): Promise => { + await cleanup(value); + }; + }); + // Let the initial setup process + await vi.advanceTimersByTimeAsync(120); + expect(setup).toHaveBeenCalledTimes(1); + expect(cleanup).toHaveBeenCalledTimes(0); + + // Send next value + behavior$.next(1); + await vi.advanceTimersByTimeAsync(50); + // Should not have started setup for 1 yet + expect(setup).toHaveBeenCalledTimes(1); + expect(cleanup).toHaveBeenCalledTimes(1); + expect(cleanup).toHaveBeenCalledWith(0); + + // Let cleanup finish + await vi.advanceTimersByTimeAsync(50); + // Now setup for 1 should have started + expect(setup).toHaveBeenCalledTimes(2); + }); + + it("should skip intermediates values that are not setup", async () => { + vi.useFakeTimers(); + const scope = new ObservableScope(); + const behavior$ = new BehaviorSubject(0); + + const setup = vi + .fn() + .mockImplementation(async (n: number) => await sleep(100)); + + const cleanupLock = Promise.withResolvers(); + const cleanup = vi + .fn() + .mockImplementation(async (n: number) => await cleanupLock.promise); + + scope.reconcile(behavior$, async (value) => { + await setup(value); + return async (): Promise => { + await cleanup(value); + }; + }); + // Let the initial setup process (0) + await vi.advanceTimersByTimeAsync(120); + + // Send 4 next values quickly + behavior$.next(1); + behavior$.next(2); + behavior$.next(3); + behavior$.next(4); + + await vi.advanceTimersByTimeAsync(3000); + // should have only called cleanup for 0 + expect(cleanup).toHaveBeenCalledTimes(1); + expect(cleanup).toHaveBeenCalledWith(0); + // Let cleanup finish + cleanupLock.resolve(undefined); + await vi.advanceTimersByTimeAsync(120); + + // Now setup for 4 should have started, skipping 1,2,3 + expect(setup).toHaveBeenCalledTimes(2); + expect(setup).toHaveBeenCalledWith(4); + expect(setup).not.toHaveBeenCalledWith(1); + expect(setup).not.toHaveBeenCalledWith(2); + expect(setup).not.toHaveBeenCalledWith(3); + }); +}); From c6bfda94cff76c900cc812e54bf8025362c97091 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 28 Nov 2025 09:53:18 +0100 Subject: [PATCH 9/9] test: additional reconcile test --- src/state/ObservableScope.test.ts | 45 +++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/state/ObservableScope.test.ts b/src/state/ObservableScope.test.ts index a0c71268..31728f39 100644 --- a/src/state/ObservableScope.test.ts +++ b/src/state/ObservableScope.test.ts @@ -191,4 +191,49 @@ describe("Reconcile", () => { expect(setup).not.toHaveBeenCalledWith(2); expect(setup).not.toHaveBeenCalledWith(3); }); + + it("should wait for setup to complete before starting cleanup", async () => { + vi.useFakeTimers(); + const scope = new ObservableScope(); + const behavior$ = new BehaviorSubject(0); + + const setup = vi + .fn() + .mockImplementation(async (n: number) => await sleep(3000)); + + const cleanupLock = Promise.withResolvers(); + const cleanup = vi + .fn() + .mockImplementation(async (n: number) => await cleanupLock.promise); + + scope.reconcile(behavior$, async (value) => { + await setup(value); + return async (): Promise => { + await cleanup(value); + }; + }); + + await vi.advanceTimersByTimeAsync(500); + // Setup for 0 should be in progress + expect(setup).toHaveBeenCalledTimes(1); + + behavior$.next(1); + await vi.advanceTimersByTimeAsync(500); + + // Should not have started setup for 1 yet + expect(setup).not.toHaveBeenCalledWith(1); + // Should not have called cleanup yet, because the setup for 0 is not done + expect(cleanup).toHaveBeenCalledTimes(0); + + // Let setup for 0 finish + await vi.advanceTimersByTimeAsync(2500 + 100); + // Now cleanup for 0 should have started + expect(cleanup).toHaveBeenCalledTimes(1); + expect(cleanup).toHaveBeenCalledWith(0); + + cleanupLock.resolve(undefined); + await vi.advanceTimersByTimeAsync(100); + // Now setup for 1 should have started + expect(setup).toHaveBeenCalledWith(1); + }); });