Conversation
Screenshot ChangesBase: Changed (144) |
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce flakiness in the component fixture/screenshot pipeline by making fixture rendering more deterministic (scheduler control + disabling animations) and ensuring required assets/services are available during CI screenshot runs.
Changes:
- Add global fixture CSS to disable animations/transitions during component rendering.
- Refactor fixture render scheduling/wait logic using
TimeTravelScheduler+ newAsyncSchedulerProcessor.waitFor(...). - Adjust fixture service stubbing and CI workflow (codicon font) to prevent runtime/asset-related instability.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/test/browser/componentFixtures/fixtureUtils.ts | Imports fixture CSS, tweaks render scheduling/waiting, and adds a stub createModelReference implementation. |
| src/vs/workbench/test/browser/componentFixtures/fixtures.css | Introduces styles intended to disable animations/transitions in fixtures. |
| src/vs/workbench/test/browser/componentFixtures/chat/chatInput.fixture.ts | Updates service wiring for the chat input fixture (moves stubbing responsibility, adds IFileDialogService). |
| src/vs/base/test/common/timeTravelScheduler.ts | Refactors scheduler processor internals and adds waitFor(...) timeout helper. |
| .github/workflows/screenshot-test.yml | Copies codicon.ttf into the expected source location before transpiling/running screenshots. |
Copilot's findings
Comments suppressed due to low confidence (3)
src/vs/workbench/test/browser/componentFixtures/fixtureUtils.ts:660
installFakeRunWhenIdle(...)is added todisposableStore, but theTimeTravelScheduler+AsyncSchedulerProcessorbacking it are disposed viaschedulerStore.dispose()right afteractualRender(). After that point, anyrunWhenIdlecallbacks scheduled by components will enqueue work onto a scheduler that is no longer being processed, which can leave parts of the UI permanently stale or leak queued tasks. Either keep the scheduler/processor alive for the fixture lifetime, or dispose/restore the fakerunWhenIdletogether withschedulerStore(e.g. register the fakerunWhenIdledisposable inschedulerStore).
This issue also appears in the following locations of the same file:
- line 662
- line 662
schedulerStore.add(scheduler.installGlobally());
disposableStore.add(installFakeRunWhenIdle((_targetWindow, callback, _timeout?) => {
return scheduler.schedule({
time: scheduler.now,
run: () => {
const deadline: IdleDeadline = {
didTimeout: true,
timeRemaining: () => 50,
};
callback(deadline);
},
source: {
toString() { return 'runWhenIdle'; },
stackTrace: undefined,
},
});
}));
src/vs/workbench/test/browser/componentFixtures/fixtureUtils.ts:673
- The previous overall render timeout guard was removed.
p.waitFor(1000)only times out the scheduler queue wait; ifoptions.render(...)returns a Promise that hangs (or awaits real timers/events), thePromise.all([...])can now block indefinitely and hang the screenshot run/CI job. Consider restoring an overall timeout around the combined render work (e.g. race the wholeactualRender()/Promise.allagainst a real-time timeout) so failures fail fast instead of hanging.
const result = options.render({ container, disposableStore, theme });
const p2 = p.waitFor(1000);
await Promise.all([
result instanceof Promise ? result : Promise.resolve(),
p2,
]);
}
await actualRender();
src/vs/workbench/test/browser/componentFixtures/fixtureUtils.ts:669
result instanceof Promiseis a brittle way to detect async renders (it won’t await non-native thenables, and it’s unnecessary here). Usingawait result(works for bothvoidand thenables) orPromise.resolve(result)would make this more robust and simpler.
const result = options.render({ container, disposableStore, theme });
const p2 = p.waitFor(1000);
await Promise.all([
result instanceof Promise ? result : Promise.resolve(),
p2,
]);
- Files reviewed: 5/5 changed files
- Comments generated: 3
| waitFor(virtualTimeMs: number): Promise<void> { | ||
| return Promise.race([ | ||
| this.waitForEmptyQueue(), | ||
| new Promise<void>((_resolve, reject) => | ||
| originalGlobalValues.setTimeout(() => { | ||
| const lastTasks = this._history.slice(-10).map(h => `${h.source.toString()}: ${h.source.stackTrace}`); | ||
| reject(new Error(`waitFor timed out after ${virtualTimeMs}ms (processed ${this._history.length} tasks, ${this.scheduler.getScheduledTasks().length} pending). These are the last ${lastTasks.length} scheduled tasks:\n${lastTasks.join('\n\n\n')}`)); | ||
| }, virtualTimeMs) |
There was a problem hiding this comment.
The parameter name virtualTimeMs is misleading here because waitFor uses a real setTimeout to enforce a wall-clock timeout (it doesn’t advance/measure the TimeTravelScheduler's virtual time). Consider renaming to timeoutMs (or similar) to make the behavior clear to callers.
| waitFor(virtualTimeMs: number): Promise<void> { | |
| return Promise.race([ | |
| this.waitForEmptyQueue(), | |
| new Promise<void>((_resolve, reject) => | |
| originalGlobalValues.setTimeout(() => { | |
| const lastTasks = this._history.slice(-10).map(h => `${h.source.toString()}: ${h.source.stackTrace}`); | |
| reject(new Error(`waitFor timed out after ${virtualTimeMs}ms (processed ${this._history.length} tasks, ${this.scheduler.getScheduledTasks().length} pending). These are the last ${lastTasks.length} scheduled tasks:\n${lastTasks.join('\n\n\n')}`)); | |
| }, virtualTimeMs) | |
| waitFor(timeoutMs: number): Promise<void> { | |
| return Promise.race([ | |
| this.waitForEmptyQueue(), | |
| new Promise<void>((_resolve, reject) => | |
| originalGlobalValues.setTimeout(() => { | |
| const lastTasks = this._history.slice(-10).map(h => `${h.source.toString()}: ${h.source.stackTrace}`); | |
| reject(new Error(`waitFor timed out after ${timeoutMs}ms (processed ${this._history.length} tasks, ${this.scheduler.getScheduledTasks().length} pending). These are the last ${lastTasks.length} scheduled tasks:\n${lastTasks.join('\n\n\n')}`)); | |
| }, timeoutMs) |
No description provided.