Skip to content

Commit 9f97549

Browse files
edrioukCopilot
andauthored
Refactor RPC calls (#199)
* Refactor RPC calls * file onDidCbuildCompleted * common result data * Run cbuild async * Wire cbuild complete to solution ,anager * Tests updates * Add tests * remove LoadPacks and getVersion calls from components-packs-webview-main.ts * Improved getVersion + tests * Update src/solutions/solution-problems.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix problems view * Method renamed to reflect its purpose * lock components-packs-webview-main.ts while converting * Rearrange RPC calls * add unit tests * Notify statusbar when cbuild completed * Remove triggerReload from ActiveSolutionTracker interface * remove redundant mock, reuse existing one * Update src/solutions/solution-manager.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Improve output messages and stop spin earlier * Remove CompileCommandsGenerator interface, run cbuild asynchronously Move severity calculation to solution-problems.ts --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 6af82d4 commit 9f97549

28 files changed

Lines changed: 853 additions & 332 deletions

src/desktop/extension.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ import { getCreateSolutionFromDataManager } from '../solutions/create-solution-f
3232
import { DebugHardwareCommands } from '../solutions/hardware/debug-hardware-commands';
3333
import { TargetPackCommandImpl } from '../solutions/hardware/target-pack-command';
3434
import { ArmclangDefineGetter } from '../solutions/intellisense/armclang-define-getter';
35-
import { CompileCommandsGeneratorImpl } from '../solutions/intellisense/compile-commands-generator';
35+
import { CompileCommandsGenerator } from '../solutions/intellisense/compile-commands-generator';
3636
import { CompileCommandsParser } from '../solutions/intellisense/compile-commands-parser';
3737
import { SolutionLanguageFeaturesProvider } from '../solutions/language-features/solution-language-features-provider';
3838
import { ConvertMdkToCsolutionCommand } from '../solutions/mdk-conversion/convert-mdk-command';
@@ -170,14 +170,13 @@ export const activate = async (context: ExtensionContext): Promise<CsolutionExte
170170

171171
const buildTaskProvider = new BuildTaskProviderImpl(buildRunner);
172172
const buildTaskDefinitionBuilder = new BuildTaskDefinitionBuilderImpl(solutionManager, configurationProvider);
173-
const compileCommandsGenerator = new CompileCommandsGeneratorImpl(buildTaskProvider, buildTaskDefinitionBuilder);
173+
const compileCommandsGenerator = new CompileCommandsGenerator(buildTaskProvider, buildTaskDefinitionBuilder, eventHub, outputChannelProvider);
174174

175175
const solutionConverterImpl = new SolutionConverterImpl(
176176
eventHub,
177177
configurationProvider,
178178
outputChannelProvider,
179179
cmsisToolboxManager,
180-
compileCommandsGenerator,
181180
);
182181

183182
const solutionProblems = new SolutionProblemsImpl(solutionManager, eventHub);
@@ -221,7 +220,7 @@ export const activate = async (context: ExtensionContext): Promise<CsolutionExte
221220
const fileDecorationProviderManager = new FileDecorationProviderManagerImpl();
222221
const treeViewProviderImpl = new TreeViewProviderImpl(SolutionOutlineView.treeViewId);
223222
const treeViewFileDecorationProvider = new TreeViewFileDecorationProvider(fileDecorationProviderManager, themeProvider);
224-
const mergeCommand = new MergeCommand(commandsProvider, activeSolutionTracker);
223+
const mergeCommand = new MergeCommand(commandsProvider);
225224
const buildCommand = new BuildCommand(buildTaskProvider, commandsProvider, buildTaskDefinitionBuilder);
226225
const runGeneratorCommand = new GeneratorCommand(commandsProvider, solutionManager, outputChannelProvider, cmsisToolboxManager);
227226
const armclangDefineGetter = new ArmclangDefineGetter(processManager, workspaceFsProvider);
@@ -265,6 +264,7 @@ export const activate = async (context: ExtensionContext): Promise<CsolutionExte
265264
runGeneratorCommand.activate(context),
266265
convertMdkToCsolution.activate(context),
267266
solutionConverterImpl.activate(context),
267+
compileCommandsGenerator.activate(context),
268268
solutionProblems.activate(context),
269269
clangdManager.activate(context),
270270
componentsManager.activate(context),

src/json-rpc/csolution-rpc-client.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import * as manifest from '../manifest';
99
import { ChildProcess, spawn } from 'node:child_process';
1010
import { MessageConnection } from 'vscode-jsonrpc';
1111
import { createMessageConnection, StreamMessageReader, StreamMessageWriter } from 'vscode-jsonrpc/node';
12-
import { RpcMethods, RpcInterface } from './interface/rpc-interface';
12+
import { RpcMethods, RpcInterface, GetVersionResult } from './interface/rpc-interface';
1313
import { constructor } from '../generic/constructor';
1414
import { Optional } from '../generic/type-helper';
1515
import { debounce } from 'lodash';
@@ -43,6 +43,7 @@ class CsolutionServiceImpl extends RpcMethods implements CsolutionService {
4343
private readonly debouncedLoadPacks = debounce(super.loadPacks.bind(this), 1000);
4444
private csolutionBin = 'csolution';
4545
private exitPromise: Promise<void> | undefined;
46+
private cachedVersion: GetVersionResult = { success: false };
4647
private readonly mutex: Mutex;
4748

4849
constructor(
@@ -71,10 +72,22 @@ class CsolutionServiceImpl extends RpcMethods implements CsolutionService {
7172
return (response ?? {}) as TResponse;
7273
}
7374

75+
public async getVersion(): Promise<GetVersionResult> {
76+
if (!this.cachedVersion.success) {
77+
// Query daemon version once per launched session
78+
this.cachedVersion = await super.getVersion();
79+
console.log('csolution version:', this.cachedVersion);
80+
}
81+
return this.cachedVersion;
82+
}
83+
7484
public async loadPacks() {
7585
if (this.idxWatcher === undefined) {
7686
this.watchPackIdxFile();
7787
}
88+
// ensure version is cached
89+
await this.getVersion();
90+
7891
return super.loadPacks();
7992
}
8093

@@ -131,6 +144,7 @@ class CsolutionServiceImpl extends RpcMethods implements CsolutionService {
131144
this.idxWatcher = undefined;
132145
this.connection?.dispose();
133146
this.connection = undefined;
147+
this.cachedVersion = { success: false };
134148
}
135149

136150
private async launch(): Promise<boolean> {

src/json-rpc/csolution-rpc-helper.test.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,66 @@ describe('csolution-rpc-client', () => {
207207
});
208208
});
209209

210+
describe('csolution-rpc-client getVersion cache', () => {
211+
let service: AnyService;
212+
213+
beforeEach(() => {
214+
service = createService();
215+
service.cachedVersion = { success: false };
216+
});
217+
218+
it('caches version after first request and reuses it', async () => {
219+
const transceiveSpy = jest.spyOn(service as any, 'transceive')
220+
.mockResolvedValue({ success: true, version: '1.2.3', apiVersion: '0.0.9' });
221+
222+
const first = await service.getVersion();
223+
const second = await service.getVersion();
224+
225+
expect(first).toEqual({ success: true, version: '1.2.3', apiVersion: '0.0.9' });
226+
expect(second).toEqual({ success: true, version: '1.2.3', apiVersion: '0.0.9' });
227+
expect(transceiveSpy).toHaveBeenCalledTimes(1);
228+
expect(transceiveSpy).toHaveBeenCalledWith('GetVersion', undefined);
229+
});
230+
231+
it('resets cached version on terminate and fetches again on next getVersion', async () => {
232+
const transceiveSpy = jest.spyOn(service as any, 'transceive')
233+
.mockResolvedValue({ success: true, version: '1.2.3', apiVersion: '0.0.9' });
234+
235+
await service.getVersion();
236+
expect(transceiveSpy).toHaveBeenCalledTimes(1);
237+
238+
// Simulate daemon termination lifecycle.
239+
service.onTerminate();
240+
expect(service.cachedVersion).toEqual({ success: false });
241+
242+
await service.getVersion();
243+
expect(transceiveSpy).toHaveBeenCalledTimes(2);
244+
});
245+
246+
it('primes version cache in loadPacks before LoadPacks RPC', async () => {
247+
service.idxWatcher = { close: jest.fn() };
248+
const sequence: string[] = [];
249+
250+
const transceiveSpy = jest.spyOn(service as any, 'transceive').mockImplementation(async (...args: unknown[]) => {
251+
const method = String(args[0]);
252+
sequence.push(method);
253+
if (method === 'GetVersion') {
254+
return { success: true, version: '1.2.3', apiVersion: '0.0.9' };
255+
}
256+
if (method === 'LoadPacks') {
257+
return { success: true };
258+
}
259+
return { success: false };
260+
});
261+
262+
await service.loadPacks();
263+
264+
expect(transceiveSpy).toHaveBeenCalledWith('GetVersion', undefined);
265+
expect(transceiveSpy).toHaveBeenCalledWith('LoadPacks', undefined);
266+
expect(sequence.indexOf('GetVersion')).toBeLessThan(sequence.indexOf('LoadPacks'));
267+
});
268+
});
269+
210270

211271
describe('csolution-rpc-client watchPackIdxFile', () => {
212272
let service: AnyService;

src/manifest.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ export const CONFIG_AUTO_SHOW_CMSIS_VIEW = 'autoShowCMSISView';
4646
export const CONFIG_BUILD_OUTPUT_VERBOSITY = 'buildOutputVerbosity';
4747
export const MANAGE_COMPONENTS_PACKS_COMMAND_ID = `${PACKAGE_NAME}.manageComponentsPacks`;
4848
export const MERGE_FILE_COMMAND_ID = `${PACKAGE_NAME}.mergeFile`;
49+
export const REFRESH_COMMAND_ID = `${PACKAGE_NAME}.refresh`;
4950
export const RUN_GENERATOR_COMMAND_ID = `${PACKAGE_NAME}.runGenerator`;
5051

5152
export const MIN_TOOLBOX_VERSION = '2.12.0';

src/solutions/active-solution-tracker.factories.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,6 @@ export const activeSolutionTrackerFactory = makeFactory<MockActiveSolutionTracke
6969
solutions: () => [],
7070
activate: () => jest.fn(),
7171
getSolutionDetails: () => jest.fn(),
72-
triggerReload: (r) => jest.fn(() => r.onActiveSolutionFilesChangedEmitter?.fire()),
7372
suspendWatch: () => false,
7473
mockFireActiveSolutionFilesChanged: (r) => () => r.onActiveSolutionFilesChangedEmitter?.fire(),
7574
});

src/solutions/active-solution-tracker.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,6 @@ export interface ActiveSolutionTracker {
8383

8484
getSolutionDetails(solutionPath: string): SolutionDetails;
8585

86-
triggerReload(): void;
87-
8886
suspendWatch: boolean;
8987
}
9088

@@ -297,7 +295,7 @@ export class ActiveSolutionTrackerImpl implements ActiveSolutionTracker {
297295
return this.configurationProvider.getConfigVariable<string>(manifest.CONFIG_EXCLUDE) || undefined;
298296
}
299297

300-
public triggerReload(): void {
298+
private triggerReload(): void {
301299
this.activeSolutionFilesChangedEmitter.fire();
302300
}
303301

src/solutions/clangd-manager.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ describe('ClangdManagerTest', () => {
6969
Add:
7070
- '-DDEBUG'
7171
---
72-
If:
72+
If:
7373
PathMatch: 'Release'
7474
CompileFlags:
7575
CompilationDatabase: >-

src/solutions/clangd-manager.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ export class ClangdManager {
9292

9393
public async activate(context: ExtensionContext): Promise<void> {
9494
this.context = context;
95-
this.solutionManager.onUpdatedCompileCommands(this.loadedBuildFiles, this, context.subscriptions);
95+
this.solutionManager.onUpdatedCompileCommands(this.updateGlobalContext, this, context.subscriptions);
9696
this.configurationProvider.onChangeConfiguration(this.debouncedUpdateClangdConfig, CONFIG_CLANGD_GENERATE_SETUP);
9797
this.commandsProvider.registerCommand(ClangdManager.setClangdContextCommandId, this.setGlobalContext, this);
9898
this.commandsProvider.registerCommand(ClangdManager.unsetClangdContextCommandId, this.unsetGlobalContext, this);
@@ -181,7 +181,7 @@ export class ClangdManager {
181181
this.context?.workspaceState.update(clangDActiveContextKey, state);
182182
}
183183

184-
private loadedBuildFiles() {
184+
private updateGlobalContext() {
185185
const csolution = this.solutionManager.getCsolution();
186186
const solutionPath = csolution?.solutionPath;
187187
if (!solutionPath) {

src/solutions/cmsis-toolbox.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,8 +180,11 @@ export class CmsisToolboxManagerImpl implements CmsisToolboxManager {
180180
console.log(msg);
181181
onOutput(msg);
182182
}
183-
// set 'packs' event flag when installing packs via 'cpackget add' command
184-
this.runCmsisToolEmitter.fire([true, tool === 'cpackget' && args.includes('add')]);
183+
184+
if (tool !== 'cbuild') { // do not notify cbuild is started
185+
// set 'packs' event flag when installing packs via 'cpackget add' command
186+
this.runCmsisToolEmitter.fire([true, tool === 'cpackget' && args.includes('add')]);
187+
}
185188

186189
const [cmdMsg, returnCode] = await this.runCmd(
187190
toolCmd,

src/solutions/intellisense/compile-commands-generator.test.ts

Lines changed: 72 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,22 +17,37 @@
1717
import { expect, it, describe } from '@jest/globals';
1818
import * as vscode from 'vscode';
1919
import * as path from 'path';
20-
import { CompileCommandsGeneratorImpl } from './compile-commands-generator';
20+
import { CompileCommandsGenerator } from './compile-commands-generator';
2121
import { buildTaskDefinitionBuilderFactory, MockBuildTaskDefinitionBuilder } from '../../tasks/build/build-task-definition-builder.factories';
2222
import { BuildTaskDefinition } from '../../tasks/build/build-task-definition';
2323
import { BuildTaskProvider, BuildTaskProviderImpl } from '../../tasks/build/build-task-provider';
24+
import { SolutionEventHub } from '../solution-event-hub';
25+
import * as manifest from '../../manifest';
26+
import { MockOutputChannelProvider, outputChannelProviderFactory } from '../../vscode-api/output-channel-provider.factories';
27+
import { waitTimeout } from '../../__test__/test-waits';
2428

2529
jest.mock('which', () => jest.fn((cmd) => Promise.resolve(path.join('path', 'to', cmd))));
2630

2731
describe('CompileCommandsGenerator', () => {
28-
let generator: CompileCommandsGeneratorImpl;
32+
let generator: CompileCommandsGenerator;
2933
let mockBuildTaskProvider: BuildTaskProvider;
3034
let buildTaskDefinitionBuilder: MockBuildTaskDefinitionBuilder;
3135
let mockExecution: vscode.TaskExecution;
3236
let exitCode: number;
37+
let mockEventHub: jest.Mocked<SolutionEventHub>;
38+
let outputChannelProvider: MockOutputChannelProvider;
39+
let cbuildSetupRequestedListener: (() => void) | undefined;
3340

3441
beforeEach(async () => {
3542
exitCode = 0;
43+
mockEventHub = {
44+
fireCbuildCompleted: jest.fn().mockResolvedValue(undefined),
45+
onDidCbuildSetupRequested: jest.fn((listener: () => void) => {
46+
cbuildSetupRequestedListener = listener;
47+
return { dispose: jest.fn() } as unknown as vscode.Disposable;
48+
}),
49+
} as unknown as jest.Mocked<SolutionEventHub>;
50+
outputChannelProvider = outputChannelProviderFactory();
3651

3752
const buildTaskDefinition: BuildTaskDefinition = {
3853
type: BuildTaskProviderImpl.taskType,
@@ -59,10 +74,13 @@ describe('CompileCommandsGenerator', () => {
5974
return { dispose: jest.fn() } as unknown as vscode.Disposable;
6075
};
6176

62-
generator = new CompileCommandsGeneratorImpl(
77+
generator = new CompileCommandsGenerator(
6378
mockBuildTaskProvider,
6479
buildTaskDefinitionBuilder,
80+
mockEventHub,
81+
outputChannelProvider,
6582
);
83+
generator.activate({ subscriptions: [] } as unknown as vscode.ExtensionContext);
6684

6785
});
6886

@@ -71,18 +89,61 @@ describe('CompileCommandsGenerator', () => {
7189
getOutputBuffer: () => ['completed with exit code 0']
7290
});
7391
exitCode = 0;
74-
const [result, output] = await generator.runCbuildSetup();
75-
expect(result).toBe(true);
76-
expect(output).toEqual(expect.arrayContaining(['completed with exit code 0']));
77-
}, 10000);
92+
cbuildSetupRequestedListener?.();
93+
await waitTimeout();
94+
95+
const outputChannel = outputChannelProvider.mockGetCreatedChannelByName(manifest.CMSIS_SOLUTION_OUTPUT_CHANNEL);
96+
expect(vscode.tasks.executeTask).toHaveBeenCalledTimes(1);
97+
expect(outputChannel!.appendLine).toHaveBeenCalledWith('Launching cbuild setup in Terminal to generate IntelliSense database');
98+
expect(mockEventHub.fireCbuildCompleted).toHaveBeenCalledWith({ success: true, severity: 'success', toolsOutputMessages: ['completed with exit code 0'] });
99+
});
100+
101+
it('runs cbuild setup when cbuild setup request event is received', async () => {
102+
(mockBuildTaskProvider.getActiveTaskRunner as jest.Mock).mockReturnValue({
103+
getOutputBuffer: () => ['completed with exit code 0']
104+
});
105+
exitCode = 0;
106+
cbuildSetupRequestedListener?.();
107+
await waitTimeout();
108+
109+
expect(vscode.tasks.executeTask).toHaveBeenCalledTimes(1);
110+
});
78111

79112
it('prints an error message if the compilation database could not be generated', async () => {
80113
(mockBuildTaskProvider.getActiveTaskRunner as jest.Mock).mockReturnValue({
81114
getOutputBuffer: () => ['failed with exit code 1']
82115
});
83116
exitCode = 1;
84-
const [result, output] = await generator.runCbuildSetup();
85-
expect(result).toBe(false);
86-
expect(output).toEqual(expect.arrayContaining(['failed with exit code 1']));
87-
}, 10000);
117+
cbuildSetupRequestedListener?.();
118+
await waitTimeout();
119+
expect(mockEventHub.fireCbuildCompleted).toHaveBeenCalledWith({ success: false, severity: 'error', toolsOutputMessages: ['failed with exit code 1'] });
120+
});
121+
122+
it('reports warning severity when exit code is 0 but output contains warning pattern', async () => {
123+
(mockBuildTaskProvider.getActiveTaskRunner as jest.Mock).mockReturnValue({
124+
getOutputBuffer: () => ['warning cbuild: deprecated option', 'completed with exit code 0']
125+
});
126+
exitCode = 0;
127+
cbuildSetupRequestedListener?.();
128+
await waitTimeout();
129+
expect(mockEventHub.fireCbuildCompleted).toHaveBeenCalledWith({
130+
success: true,
131+
severity: 'warning',
132+
toolsOutputMessages: ['warning cbuild: deprecated option', 'completed with exit code 0']
133+
});
134+
});
135+
136+
it('reports error severity when exit code is 0 but output contains error pattern', async () => {
137+
(mockBuildTaskProvider.getActiveTaskRunner as jest.Mock).mockReturnValue({
138+
getOutputBuffer: () => ['error cbuild: failed to generate', 'completed with exit code 0']
139+
});
140+
exitCode = 0;
141+
cbuildSetupRequestedListener?.();
142+
await waitTimeout();
143+
expect(mockEventHub.fireCbuildCompleted).toHaveBeenCalledWith({
144+
success: true,
145+
severity: 'error',
146+
toolsOutputMessages: ['error cbuild: failed to generate', 'completed with exit code 0']
147+
});
148+
});
88149
});

0 commit comments

Comments
 (0)