From 67e0d937e2c342a905f4d827bccd3912d6d2a356 Mon Sep 17 00:00:00 2001 From: Akos Kitta Date: Tue, 22 Aug 2023 13:17:22 +0200 Subject: [PATCH 1/2] fix: can unset `network#proxy` in the CLI config An empty object (`{}`) must be used to correctly unset the CLI config value to its default. Closes #2184 --- .../src/node/config-service-impl.ts | 2 +- .../node/config-service-impl.slow-test.ts | 116 ++++++++++++++++++ 2 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 arduino-ide-extension/src/test/node/config-service-impl.slow-test.ts diff --git a/arduino-ide-extension/src/node/config-service-impl.ts b/arduino-ide-extension/src/node/config-service-impl.ts index 1f56748cc..05e1babf0 100644 --- a/arduino-ide-extension/src/node/config-service-impl.ts +++ b/arduino-ide-extension/src/node/config-service-impl.ts @@ -97,7 +97,7 @@ export class ConfigServiceImpl }; copyDefaultCliConfig.locale = locale || 'en'; const proxy = Network.stringify(network); - copyDefaultCliConfig.network = { proxy }; + copyDefaultCliConfig.network = proxy ? { proxy } : {}; // must be an empty object to unset the default prop with the `WriteRequest`. // always use the port of the daemon const port = await this.daemon.getPort(); diff --git a/arduino-ide-extension/src/test/node/config-service-impl.slow-test.ts b/arduino-ide-extension/src/test/node/config-service-impl.slow-test.ts new file mode 100644 index 000000000..2a50a9a7b --- /dev/null +++ b/arduino-ide-extension/src/test/node/config-service-impl.slow-test.ts @@ -0,0 +1,116 @@ +import { DisposableCollection } from '@theia/core/lib/common/disposable'; +import { deepClone } from '@theia/core/lib/common/objects'; +import type { MaybePromise, Mutable } from '@theia/core/lib/common/types'; +import { Container } from '@theia/core/shared/inversify'; +import { expect } from 'chai'; +import { load as parseYaml } from 'js-yaml'; +import { promises as fs } from 'node:fs'; +import { Config, Network } from '../../common/protocol/config-service'; +import { CLI_CONFIG, DefaultCliConfig } from '../../node/cli-config'; +import { ConfigServiceImpl } from '../../node/config-service-impl'; +import { ConfigDirUriProvider } from '../../node/theia/env-variables/env-variables-server'; +import { createBaseContainer, startDaemon } from './node-test-bindings'; + +describe('config-service-impl', () => { + describe('setConfiguration', () => { + const manualProxy: Network = { + protocol: 'http', + hostname: 'hostname', + password: 'secret', + username: 'username', + port: '1234', + }; + const noProxy: Network = 'none'; + + let configService: ConfigServiceImpl; + let toDispose: DisposableCollection; + let cliConfigPath: string; + + beforeEach(async () => { + const container = await createContainer(); + toDispose = new DisposableCollection(); + await startDaemon(container, toDispose); + configService = container.get(ConfigServiceImpl); + const configDirUriProvider = + container.get(ConfigDirUriProvider); + cliConfigPath = configDirUriProvider + .configDirUri() + .resolve(CLI_CONFIG) + .path.fsPath(); + }); + + afterEach(() => toDispose.dispose()); + + it("should detect 'none' proxy with th default config", async () => { + const state = await configService.getConfiguration(); + expect(state.config).to.be.not.undefined; + const config = state.config; + expect(config.network).to.be.equal(noProxy); + expect(Network.stringify(config.network)).is.undefined; + await assertRawConfigModel((actualModel) => { + expect(actualModel.network).to.be.undefined; + }); + }); + + it('should ignore noop changes', async () => { + const beforeState = await configService.getConfiguration(); + const config = >deepClone(beforeState).config; + let eventCounter = 0; + toDispose.push(configService.onConfigChange(() => eventCounter++)); + await configService.setConfiguration(config); + const afterState = await configService.getConfiguration(); + expect(beforeState.config).to.be.deep.equal(afterState.config); + expect(eventCounter).to.be.equal(0); + }); + + it('should set the manual proxy', async () => { + const beforeState = await configService.getConfiguration(); + const config = >deepClone(beforeState).config; + config.network = manualProxy; + let eventCounter = 0; + toDispose.push(configService.onConfigChange(() => eventCounter++)); + await configService.setConfiguration(config); + const afterState = await configService.getConfiguration(); + expect(beforeState.config).to.be.not.deep.equal(afterState.config); + expect(afterState.config?.network).to.be.deep.equal(manualProxy); + expect(eventCounter).to.be.equal(1); + await assertRawConfigModel((actualModel) => { + expect(actualModel.network?.proxy).to.be.equal( + Network.stringify(manualProxy) + ); + }); + }); + + it('should unset the manual proxy', async () => { + const initialState = await configService.getConfiguration(); + const config = >deepClone(initialState).config; + config.network = manualProxy; + let eventCounter = 0; + toDispose.push(configService.onConfigChange(() => eventCounter++)); + await configService.setConfiguration(config); + const beforeState = await configService.getConfiguration(); + const config2 = >deepClone(config); + config2.network = noProxy; + await configService.setConfiguration(config2); + const afterState = await configService.getConfiguration(); + expect(beforeState.config).to.be.not.deep.equal(afterState.config); + expect(afterState.config?.network).to.be.deep.equal(noProxy); + expect(eventCounter).to.be.equal(2); + await assertRawConfigModel((actualModel) => { + expect(actualModel.network?.proxy).to.be.undefined; + }); + }); + + async function createContainer(): Promise { + return createBaseContainer(); + } + + async function assertRawConfigModel( + assert: (actual: DefaultCliConfig) => MaybePromise + ): Promise { + const raw = await fs.readFile(cliConfigPath, { encoding: 'utf8' }); + const model = parseYaml(raw); + await assert(model); + } + }); +}); From b935f576afafe70a9b796670dad8daffe4304a2b Mon Sep 17 00:00:00 2001 From: Akos Kitta Date: Wed, 23 Aug 2023 14:35:51 +0200 Subject: [PATCH 2/2] test: added test case for the bug --- .../node/config-service-impl.slow-test.ts | 130 +++++++++++++----- 1 file changed, 96 insertions(+), 34 deletions(-) diff --git a/arduino-ide-extension/src/test/node/config-service-impl.slow-test.ts b/arduino-ide-extension/src/test/node/config-service-impl.slow-test.ts index 2a50a9a7b..fe585bb7d 100644 --- a/arduino-ide-extension/src/test/node/config-service-impl.slow-test.ts +++ b/arduino-ide-extension/src/test/node/config-service-impl.slow-test.ts @@ -1,42 +1,50 @@ -import { DisposableCollection } from '@theia/core/lib/common/disposable'; +import { + Disposable, + DisposableCollection, +} from '@theia/core/lib/common/disposable'; import { deepClone } from '@theia/core/lib/common/objects'; import type { MaybePromise, Mutable } from '@theia/core/lib/common/types'; -import { Container } from '@theia/core/shared/inversify'; +import type { Container } from '@theia/core/shared/inversify'; import { expect } from 'chai'; import { load as parseYaml } from 'js-yaml'; import { promises as fs } from 'node:fs'; -import { Config, Network } from '../../common/protocol/config-service'; +import { join } from 'node:path'; +import temp from 'temp'; +import { + Config, + Network, + ProxySettings, +} from '../../common/protocol/config-service'; import { CLI_CONFIG, DefaultCliConfig } from '../../node/cli-config'; import { ConfigServiceImpl } from '../../node/config-service-impl'; import { ConfigDirUriProvider } from '../../node/theia/env-variables/env-variables-server'; -import { createBaseContainer, startDaemon } from './node-test-bindings'; +import { + createBaseContainer, + createCliConfig, + startDaemon, +} from './node-test-bindings'; describe('config-service-impl', () => { - describe('setConfiguration', () => { - const manualProxy: Network = { - protocol: 'http', - hostname: 'hostname', - password: 'secret', - username: 'username', - port: '1234', - }; - const noProxy: Network = 'none'; + const noProxy = 'none'; + const manualProxy: ProxySettings = { + protocol: 'http', + hostname: 'hostname', + password: 'secret', + username: 'username', + port: '1234', + }; + describe('setConfiguration', () => { let configService: ConfigServiceImpl; let toDispose: DisposableCollection; let cliConfigPath: string; beforeEach(async () => { - const container = await createContainer(); + const container = await createBaseContainer(); toDispose = new DisposableCollection(); await startDaemon(container, toDispose); configService = container.get(ConfigServiceImpl); - const configDirUriProvider = - container.get(ConfigDirUriProvider); - cliConfigPath = configDirUriProvider - .configDirUri() - .resolve(CLI_CONFIG) - .path.fsPath(); + cliConfigPath = getCliConfigPath(container); }); afterEach(() => toDispose.dispose()); @@ -47,7 +55,7 @@ describe('config-service-impl', () => { const config = state.config; expect(config.network).to.be.equal(noProxy); expect(Network.stringify(config.network)).is.undefined; - await assertRawConfigModel((actualModel) => { + await assertRawConfigModel(cliConfigPath, (actualModel) => { expect(actualModel.network).to.be.undefined; }); }); @@ -74,7 +82,7 @@ describe('config-service-impl', () => { expect(beforeState.config).to.be.not.deep.equal(afterState.config); expect(afterState.config?.network).to.be.deep.equal(manualProxy); expect(eventCounter).to.be.equal(1); - await assertRawConfigModel((actualModel) => { + await assertRawConfigModel(cliConfigPath, (actualModel) => { expect(actualModel.network?.proxy).to.be.equal( Network.stringify(manualProxy) ); @@ -96,21 +104,75 @@ describe('config-service-impl', () => { expect(beforeState.config).to.be.not.deep.equal(afterState.config); expect(afterState.config?.network).to.be.deep.equal(noProxy); expect(eventCounter).to.be.equal(2); - await assertRawConfigModel((actualModel) => { + await assertRawConfigModel(cliConfigPath, (actualModel) => { expect(actualModel.network?.proxy).to.be.undefined; }); }); + }); + + describe('setConfiguration (multiple CLI daemon sessions)', () => { + let tracked: typeof temp; + let toDispose: DisposableCollection; + + before(() => { + tracked = temp.track(); + toDispose = new DisposableCollection( + Disposable.create(() => tracked.cleanupSync()) + ); + }); + + after(() => toDispose.dispose()); + + it("should unset the 'network#proxy' config value between daemon sessions", async () => { + const configDirPath = tracked.mkdirSync(); + const cliConfigPath = join(configDirPath, CLI_CONFIG); + const cliConfig = await createCliConfig(configDirPath); + const setupContainer = await createBaseContainer({ + cliConfig, + configDirPath, + }); + const toDisposeAfterFirstStart = new DisposableCollection(); + toDispose.push(toDisposeAfterFirstStart); + await startDaemon(setupContainer, toDisposeAfterFirstStart); + toDisposeAfterFirstStart.dispose(); + + // second startup when the indexes are all downloaded and the daemon is initialized with the network#proxy + cliConfig.network = { proxy: Network.stringify(manualProxy) }; + const container = await createBaseContainer({ cliConfig, configDirPath }); + await startDaemon(container, toDispose); + const configService = container.get(ConfigServiceImpl); + let eventCounter = 0; + toDispose.push(configService.onConfigChange(() => eventCounter++)); - async function createContainer(): Promise { - return createBaseContainer(); - } - - async function assertRawConfigModel( - assert: (actual: DefaultCliConfig) => MaybePromise - ): Promise { - const raw = await fs.readFile(cliConfigPath, { encoding: 'utf8' }); - const model = parseYaml(raw); - await assert(model); - } + const beforeState = await configService.getConfiguration(); + const config = >deepClone(beforeState.config); + config.network = noProxy; + await configService.setConfiguration(config); + const afterState = await configService.getConfiguration(); + expect(beforeState.config).to.be.not.deep.equal(afterState.config); + expect(afterState.config?.network).to.be.deep.equal(noProxy); + expect(eventCounter).to.be.equal(1); + await assertRawConfigModel(cliConfigPath, (actualModel) => { + expect(actualModel.network?.proxy).to.be.undefined; // currently fails due to arduino/arduino-cli#2275 + }); + }); }); + + async function assertRawConfigModel( + cliConfigPath: string, + assert: (actual: DefaultCliConfig) => MaybePromise + ): Promise { + const raw = await fs.readFile(cliConfigPath, { encoding: 'utf8' }); + const model = parseYaml(raw); + await assert(model); + } + + function getCliConfigPath(container: Container): string { + const configDirUriProvider = + container.get(ConfigDirUriProvider); + return configDirUriProvider + .configDirUri() + .resolve(CLI_CONFIG) + .path.fsPath(); + } });