config: make all settings experiment-aware by default#300437
Conversation
…tests for configuration overrides
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
@kycutlerMatched files:
@jrualesMatched files:
@jriekenMatched files:
|
There was a problem hiding this comment.
Pull request overview
This PR makes configuration defaults experiment-overridable by default, using a standardized treatment name format (config.${settingId}), and simplifies the schema surface from an opt-in experiment object to an experimentMode refresh control (startup-only vs default auto refresh).
Changes:
- Apply experiment overrides to all registered settings by default, with treatments read as
config.${settingId}. - Replace
experimentschema property withexperimentMode?: 'startup'and update built-in settings accordingly. - Force extension-contributed settings to
experimentMode: 'startup'and add new tests for default override behavior.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/services/configuration/test/browser/configurationDefaultOverrides.test.ts | Adds unit tests for experiment-driven default overrides and refresh behavior. |
| src/vs/workbench/services/configuration/browser/configurationService.ts | Removes per-setting opt-in and applies experiment processing broadly; switches refresh behavior to experimentMode. |
| src/vs/workbench/contrib/welcomeGettingStarted/browser/gettingStarted.contribution.ts | Removes now-redundant experiment: { mode: 'auto' } declaration. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts | Removes redundant experiment: { mode: 'auto' } declarations from terminal chat agent tools settings. |
| src/vs/workbench/contrib/terminal/common/terminalConfiguration.ts | Removes redundant experiment: { mode: 'auto' } declarations from terminal settings. |
| src/vs/workbench/contrib/performance/electron-browser/performance.contribution.ts | Migrates a startup-only experiment declaration to experimentMode: 'startup'. |
| src/vs/workbench/contrib/inlineChat/common/inlineChat.ts | Removes redundant auto experiment declarations; keeps startup-only via experimentMode. |
| src/vs/workbench/contrib/extensions/browser/extensions.contribution.ts | Removes redundant experiment: { mode: 'auto' } from an extensions setting. |
| src/vs/workbench/contrib/editTelemetry/browser/editTelemetry.contribution.ts | Removes redundant auto experiment declarations from edit telemetry settings. |
| src/vs/workbench/contrib/chat/browser/chat.contribution.ts | Removes redundant auto experiment declarations and migrates startup-only to experimentMode. |
| src/vs/workbench/contrib/browserView/electron-browser/browserView.contribution.ts | Migrates startup-only experiment declaration to experimentMode: 'startup'. |
| src/vs/workbench/browser/workbench.contribution.ts | Removes redundant experiment: { mode: 'auto' } declarations from workbench settings. |
| src/vs/workbench/api/common/configurationExtensionPoint.ts | Forces extension-contributed settings into startup-only experiment refresh mode. |
| src/vs/platform/request/common/request.ts | Removes redundant experiment: { mode: 'auto' } declarations from request/proxy settings. |
| src/vs/platform/configuration/common/configurationRegistry.ts | Replaces experiment schema with experimentMode and updates validation/messaging around the legacy onExP tag. |
| src/vs/editor/common/config/editorOptions.ts | Removes redundant experiment: { mode: 'auto' } declarations from editor option registrations. |
| src/vs/editor/common/config/editorConfigurationSchema.ts | Removes redundant experiment: { mode: 'auto' } declarations from editor configuration schema. |
Comments suppressed due to low confidence (3)
src/vs/workbench/services/configuration/test/browser/configurationDefaultOverrides.test.ts:167
setting without experimentMode defaults to auto and re-applies on refetchcurrently has no assertions, so it can pass even if the refetch path doesn’t actually re-apply anything. Please assert an observable outcome (e.g. check the defaults override value after first apply and after refetch, or verifygetTreatmentwas called again and the effective default override changed from 42 to 99).
test('setting without experimentMode defaults to auto and re-applies on refetch', async () => {
configurationRegistry.registerConfiguration({
id: 'test.experiments.autoDefault',
properties: {
'test.experiments.autoDefault.setting': {
type: 'number',
default: 0,
}
}
});
localDisposables.add({ dispose: () => configurationRegistry.deregisterConfigurations([{ id: 'test.experiments.autoDefault', properties: { 'test.experiments.autoDefault.setting': { type: 'number', default: 0 } } }]) });
assignmentService.setTreatment('config.test.experiments.autoDefault.setting', 42);
createContribution();
await Event.toPromise(configurationRegistry.onDidUpdateConfiguration);
// Now change the treatment and refetch
assignmentService.setTreatment('config.test.experiments.autoDefault.setting', 99);
assignmentService.fireRefetch();
await Event.toPromise(configurationRegistry.onDidUpdateConfiguration);
});
src/vs/workbench/services/configuration/test/browser/configurationDefaultOverrides.test.ts:200
- The refetch test for startup mode also uses a fixed
setTimeoutdelay. Consider asserting refetch behavior via agetTreatmentcall counter/event on the mock (startup mode should not trigger another read) so the test remains deterministic and faster.
// Now change the treatment and refetch — startup mode should NOT re-apply
let overrideRegistered = false;
const listener = localDisposables.add(configurationRegistry.onDidUpdateConfiguration(() => {
overrideRegistered = true;
}));
assignmentService.setTreatment('config.test.experiments.startupMode.setting', 99);
assignmentService.fireRefetch();
// Give the async processing time
await new Promise(resolve => setTimeout(resolve, 50));
assert.strictEqual(overrideRegistered, false, 'Startup mode setting should not be refetched');
listener.dispose();
});
src/vs/workbench/services/configuration/browser/configurationService.ts:1396
- This change adds almost every setting (anything not explicitly
experimentMode: 'startup') intoautoExperimentalSettings, which meansonDidRefetchAssignmentswill re-runprocessExperimentalSettingsover (nearly) the entire settings set. Even if only a handful of treatments are active, refetch will still do the full scan unless the set is narrowed. Consider tracking only settings that currently have an active experiment override (or deriving the list from the refetched assignment context) so refetch work scales with #active treatments rather than #settings.
this.processedExperimentalSettings.add(property);
if (schema.experimentMode !== 'startup') {
this.autoExperimentalSettings.add(property);
}
src/vs/workbench/services/configuration/test/browser/configurationDefaultOverrides.test.ts
Show resolved
Hide resolved
src/vs/workbench/services/configuration/test/browser/configurationDefaultOverrides.test.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/services/configuration/test/browser/configurationDefaultOverrides.test.ts
Show resolved
Hide resolved
| await this.processExperimentalSettings(Object.keys(this.configurationRegistry.getConfigurationProperties()), false); | ||
| } finally { | ||
| // Invalidate defaults cache after extensions have registered | ||
| // and after the experiments have been resolved to prevent | ||
| // resetting the overrides too early. | ||
| await this.extensionService.whenInstalledExtensionsRegistered(); | ||
| this.logService.trace('ConfigurationService#updateDefaults: resetting the defaults'); | ||
| this.configurationService.reloadConfiguration(ConfigurationTarget.DEFAULT); | ||
| } | ||
| } | ||
|
|
||
| private async processExperimentalSettings(properties: Iterable<string>, autoRefetch: boolean): Promise<void> { | ||
| const overrides: IStringDictionary<unknown> = {}; | ||
| const allProperties = this.configurationRegistry.getConfigurationProperties(); | ||
| const defaultConfigurationsPreventingExperimentOverrides = this.configurationRegistry.getRegisteredDefaultConfigurations().filter(configuration => configuration.preventExperimentOverride); | ||
| for (const property of properties) { | ||
| const schema = allProperties[property]; | ||
| if (!schema?.experiment) { | ||
| if (!schema) { | ||
| continue; | ||
| } | ||
| const defaultValueSource: ConfigurationDefaultSource | undefined = schema.defaultValueSource && !(schema.defaultValueSource instanceof Map) ? schema.defaultValueSource : undefined; | ||
| if (defaultValueSource && defaultConfigurationsPreventingExperimentOverrides.some(configuration => isConfigurationDefaultSourceEquals(configuration.source, defaultValueSource) && configuration.overrides?.[property] !== undefined)) { | ||
| continue; | ||
| } | ||
| if (!autoRefetch && this.processedExperimentalSettings.has(property)) { | ||
| continue; | ||
| } | ||
| this.processedExperimentalSettings.add(property); | ||
| if (schema.experiment.mode === 'auto') { | ||
| if (schema.experimentMode !== 'startup') { | ||
| this.autoExperimentalSettings.add(property); | ||
| } | ||
| try { | ||
| const value = await this.workbenchAssignmentService.getTreatment(schema.experiment.name ?? `config.${property}`); | ||
| const value = await this.workbenchAssignmentService.getTreatment(`config.${property}`); | ||
| if (!isUndefined(value) && !equals(value, schema.default)) { | ||
| overrides[property] = value; |
There was a problem hiding this comment.
processExperimentalSettings now iterates over all configuration properties and awaits workbenchAssignmentService.getTreatment for each one. In production getTreatment can block on TAS initialization/network and also logs a telemetry event per call, so this becomes an O(#settings) async loop with potentially thousands of treatment reads and telemetry logs on startup and on each assignment refetch. Consider switching to a batched approach (e.g. parse getCurrentExperiments() / assignment context once into a map of config.* treatments, or add an API to fetch all config.* variables in one shot) and only process keys that actually have a treatment/value present.
This issue also appears on line 1393 of the same file.
- Track requested treatment names in MockAssignmentService
- Assert treatment lookup uses config.{settingId} format
- Assert override is applied and refetch re-requests treatment for auto mode
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add whenTreatmentRequested/whenNextTreatmentRequested to mock - Use sentinel auto-mode setting to detect refetch completion - All tests now await specific signals instead of sleeping Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
defaultValueSource is not set when no source is provided to registerDefaultConfigurations. Use getConfigurationDefaultsOverrides() instead to verify overrides were actually registered. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Make every configuration setting experiment-aware by default, instead of requiring individual opt-in via the
experimentproperty.Changes
All settings are now experiment-aware
experimentproperty guard inConfigurationDefaultOverridesContribution— all registered settings are now processed for experiment overrides using the treatment nameconfig.${settingId}.Simplified
experiment→experimentModeexperimentproperty onIConfigurationPropertySchematoexperimentModefor clarity — it now only controls the refresh mode, not whether the setting participates in experiments.{ mode: 'startup' | 'auto'; name?: string }to just'startup'— thenamefield was unused andautois now the default.experimentMode: 'auto'declarations across ~40 setting registrations sinceautois the default mode.Extension settings default to startup mode
experimentMode: 'startup'since extensions cannot control this property and startup-only is the safe default.Tests
configurationDefaultOverrides.test.tswith tests covering:experimentModeconfig.{settingId})