fix: register Dev Container Agent Host settings via idempotent helper (build fix for vscode-engineering#3738) - #333815
Conversation
… (build fix for vscode-engineering#3738) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Extracts Dev Container Agent Host setting registration to recover from shared registry resets during unit tests.
Changes:
- Adds a reusable configuration registration helper.
- Restores missing settings during test-suite setup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
devContainerAgentHostConnector.contribution.ts |
Extracts setting registration into a helper. |
devContainerAgentHostConnector.test.ts |
Re-registers missing configuration before tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Closing as superseded by #333811 (commit
|
Build failure
The Electron unit-test job failed on Linux, Windows, and macOS with a single failing test:
The test reads
getConfigurationProperties()[DevContainerAgentHostEnabledSettingId]and the returned property isundefined, so accessing.defaultthrows. Because the unit-test job exits before writing.build/logs, downstream tasks fail only on the missing path and are not independent problems.Root cause
The Dev Container Agent Host settings were registered once, unconditionally, as a side effect of module import in
devContainerAgentHostConnector.contribution.ts. TheIConfigurationRegistryis a process-global singleton shared across the whole unit-test run. Other suites callderegisterConfigurationson that shared registry (e.g. test isolation/teardown), which removes these properties fromgetConfigurationProperties(). Once removed, they are never re-registered because the module was already imported and its top-level registration ran only once. When theregisters an experimental, disabled-by-default user settingtest then reads the property back, it is gone, andproperty.defaultthrows.This is a repository-local defect in
microsoft/vscode, still present onmain, and directly caused by the setting registration/test introduced in commitf83aa9cd(#333782).How the fix works
The top-level registration is extracted into an exported, idempotent
registerDevContainerAgentHostConfiguration()helper. The module still calls it once on import to preserve existing production behavior. The test suite adds asuiteSetupthat re-registers the configuration only when it is missing from the shared registry, making the suite resilient to registry deregistration performed by other suites. This restores the property before the assertions read it, without changing runtime behavior or suppressing any real error.Rollback evaluation
f83aa9cd40904b8a52333089558d6fe40f43fde7(Agent Host: Gate Dev Container worktree sessions #333782).maininsider. A bounded fix-forward that makes the setting registration idempotent and the test isolation-safe is smaller and safer than unwinding the four-file gating change and its dependent worktree work.@chrmartiValidation
Unable to run the unit test in this environment:
npm ci/ build tooling could not be executed here (dependency install and git/build operations were unavailable). The change is verified by source inspection: the setting is now re-registered before the suite runs whenever the sharedIConfigurationRegistryno longer contains it, sogetConfigurationProperties()[DevContainerAgentHostEnabledSettingId]is defined when the assertions execute.Risk
Low. Production behavior is unchanged (the helper is still invoked exactly once on import). The added
suiteSetuponly re-registers when the property is absent, so it cannot double-register during a normal run.Recommended reviewer
Recommended owner:
@chrmartiFixes microsoft/vscode-engineering#3738