Skip to content

Commit 7ac0553

Browse files
committed
fixup! refactor(@angular/cli): clean up MCP tests and use real project name for default projects
1 parent ec9534a commit 7ac0553

File tree

10 files changed

+185
-42
lines changed

10 files changed

+185
-42
lines changed

packages/angular/cli/src/commands/mcp/testing/test-utils.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ export function addProjectToWorkspace(
8181
projects: workspaces.ProjectDefinitionCollection,
8282
name: string,
8383
targets: Record<string, workspaces.TargetDefinition> = {},
84-
root: string = `projects/${name}`,
84+
root = `projects/${name}`,
8585
) {
8686
projects.set(name, {
8787
root,

packages/angular/cli/src/commands/mcp/tools/build.ts

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import { z } from 'zod';
1010
import { CommandError, type Host } from '../host';
11-
import { createStructuredContentOutput } from '../utils';
11+
import { createStructuredContentOutput, getCommandErrorLogs } from '../utils';
1212
import { type McpToolDeclaration, declareTool } from './tool-registry';
1313

1414
const DEFAULT_CONFIGURATION = 'development';
@@ -55,13 +55,7 @@ export async function runBuild(input: BuildToolInput, host: Host) {
5555
logs = (await host.runCommand('ng', args)).logs;
5656
} catch (e) {
5757
status = 'failure';
58-
if (e instanceof CommandError) {
59-
logs = e.logs;
60-
} else if (e instanceof Error) {
61-
logs = [e.message];
62-
} else {
63-
logs = [String(e)];
64-
}
58+
logs = getCommandErrorLogs(e);
6559
}
6660

6761
for (const line of logs) {

packages/angular/cli/src/commands/mcp/tools/build_spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ describe('Build Tool', () => {
7979
'production',
8080
]);
8181
expect(structuredContent.status).toBe('failure');
82-
expect(structuredContent.logs).toEqual(buildLogs);
82+
expect(structuredContent.logs).toEqual([...buildLogs, 'Build failed']);
8383
expect(structuredContent.path).toBeUndefined();
8484
});
8585

packages/angular/cli/src/commands/mcp/tools/e2e.ts

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,12 @@
88

99
import { z } from 'zod';
1010
import { CommandError, type Host, LocalWorkspaceHost } from '../host';
11-
import { createStructuredContentOutput, getDefaultProjectName, getProject } from '../utils';
11+
import {
12+
createStructuredContentOutput,
13+
getCommandErrorLogs,
14+
getDefaultProjectName,
15+
getProject,
16+
} from '../utils';
1217
import { type McpToolContext, type McpToolDeclaration, declareTool } from './tool-registry';
1318

1419
const e2eStatusSchema = z.enum(['success', 'failure']);
@@ -36,14 +41,16 @@ export async function runE2e(input: E2eToolInput, host: Host, context: McpToolCo
3641
const projectName = input.project ?? getDefaultProjectName(context);
3742

3843
if (context.workspace && projectName) {
44+
// Verify that if a project can be found, it has an e2e testing already set up.
3945
const targetProject = getProject(context, projectName);
40-
4146
if (targetProject) {
4247
if (!targetProject.targets.has('e2e')) {
4348
return createStructuredContentOutput({
4449
status: 'failure',
4550
logs: [
46-
`No e2e target is defined for project '${projectName}'. Please setup e2e testing first.`,
51+
`No e2e target is defined for project '${projectName}'. Please set up e2e testing` +
52+
' first by calling `ng e2e` in an interactive console.' +
53+
' See https://angular.dev/tools/cli/end-to-end.',
4754
],
4855
});
4956
}
@@ -63,13 +70,7 @@ export async function runE2e(input: E2eToolInput, host: Host, context: McpToolCo
6370
logs = (await host.runCommand('ng', args)).logs;
6471
} catch (e) {
6572
status = 'failure';
66-
if (e instanceof CommandError) {
67-
logs = e.logs;
68-
} else if (e instanceof Error) {
69-
logs = [e.message];
70-
} else {
71-
logs = [String(e)];
72-
}
73+
logs = getCommandErrorLogs(e);
7374
}
7475

7576
const structuredContent: E2eToolOutput = {
@@ -91,11 +92,12 @@ export const E2E_TOOL: McpToolDeclaration<
9192
Perform an end-to-end test with ng e2e.
9293
</Purpose>
9394
<Use Cases>
94-
* Running end-to-end tests for the project.
95+
* When the user requests running end-to-end tests for the project.
96+
* When verifying changes that cross unit boundaries, such as changes to both client and server, changes to shared data types, etc.
9597
</Use Cases>
9698
<Operational Notes>
97-
* This tool runs "ng e2e".
98-
* It will error if no "e2e" target is defined in the project, to avoid interactive setup prompts.
99+
* This tool uses "ng e2e".
100+
* Important: this relies on e2e tests being already configured for this project. It will error out if no "e2e" target is defined.
99101
</Operational Notes>
100102
`,
101103
isReadOnly: false,

packages/angular/cli/src/commands/mcp/tools/e2e_spec.ts

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,15 +68,29 @@ describe('E2E Tool', () => {
6868
expect(mockHost.runCommand).toHaveBeenCalledWith('ng', ['e2e']);
6969
});
7070

71-
it('should handle a successful e2e run', async () => {
71+
it('should handle a successful e2e run with a specified project', async () => {
7272
addProjectToWorkspace(mockProjects, 'my-app', { e2e: { builder: 'mock-builder' } });
73-
const e2eLogs = ['E2E passed'];
73+
const e2eLogs = ['E2E passed for my-app'];
7474
mockHost.runCommand.and.resolveTo({ logs: e2eLogs });
7575

7676
const { structuredContent } = await runE2e({ project: 'my-app' }, mockHost, mockContext);
7777

7878
expect(structuredContent.status).toBe('success');
7979
expect(structuredContent.logs).toEqual(e2eLogs);
80+
expect(mockHost.runCommand).toHaveBeenCalledWith('ng', ['e2e', 'my-app']);
81+
});
82+
83+
it('should handle a successful e2e run with the default project', async () => {
84+
mockWorkspace.extensions['defaultProject'] = 'default-app';
85+
addProjectToWorkspace(mockProjects, 'default-app', { e2e: { builder: 'mock-builder' } });
86+
const e2eLogs = ['E2E passed for default-app'];
87+
mockHost.runCommand.and.resolveTo({ logs: e2eLogs });
88+
89+
const { structuredContent } = await runE2e({}, mockHost, mockContext);
90+
91+
expect(structuredContent.status).toBe('success');
92+
expect(structuredContent.logs).toEqual(e2eLogs);
93+
expect(mockHost.runCommand).toHaveBeenCalledWith('ng', ['e2e']);
8094
});
8195

8296
it('should handle a failed e2e run', async () => {
@@ -87,6 +101,6 @@ describe('E2E Tool', () => {
87101
const { structuredContent } = await runE2e({ project: 'my-app' }, mockHost, mockContext);
88102

89103
expect(structuredContent.status).toBe('failure');
90-
expect(structuredContent.logs).toEqual(e2eLogs);
104+
expect(structuredContent.logs).toEqual([...e2eLogs, 'Failed']);
91105
});
92106
});

packages/angular/cli/src/commands/mcp/tools/modernize.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
import { dirname, join, relative } from 'path';
1010
import { z } from 'zod';
1111
import { CommandError, type Host } from '../host';
12-
import { createStructuredContentOutput, findAngularJsonDir } from '../utils';
12+
import { createStructuredContentOutput, findAngularJsonDir, getCommandErrorLogs } from '../utils';
1313
import { type McpToolDeclaration, declareTool } from './tool-registry';
1414

1515
interface Transformation {
@@ -152,10 +152,7 @@ export async function runModernization(input: ModernizeInput, host: Host) {
152152
`Migration ${transformation.name} on directory ${relativePath} completed successfully.`,
153153
);
154154
} catch (e) {
155-
if (e instanceof CommandError) {
156-
logs = e.logs;
157-
}
158-
logs.push((e as Error).message);
155+
logs = getCommandErrorLogs(e);
159156
instructions.push(
160157
`Migration ${transformation.name} on directory ${relativePath} failed.`,
161158
);

packages/angular/cli/src/commands/mcp/tools/test.ts

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import { z } from 'zod';
1010
import { CommandError, type Host, LocalWorkspaceHost } from '../host';
11-
import { createStructuredContentOutput } from '../utils';
11+
import { createStructuredContentOutput, getCommandErrorLogs } from '../utils';
1212
import { type McpToolDeclaration, declareTool } from './tool-registry';
1313

1414
const testStatusSchema = z.enum(['success', 'failure']);
@@ -53,13 +53,7 @@ export async function runTest(input: TestToolInput, host: Host) {
5353
logs = (await host.runCommand('ng', args)).logs;
5454
} catch (e) {
5555
status = 'failure';
56-
if (e instanceof CommandError) {
57-
logs = e.logs;
58-
} else if (e instanceof Error) {
59-
logs = [e.message];
60-
} else {
61-
logs = [String(e)];
62-
}
56+
logs = getCommandErrorLogs(e);
6357
}
6458

6559
const structuredContent: TestToolOutput = {
@@ -85,8 +79,9 @@ Perform a one-off, non-watched unit test execution with ng test.
8579
* Verifying code changes with tests.
8680
</Use Cases>
8781
<Operational Notes>
88-
* This tool runs "ng test" with "--watch false".
82+
* This tool uses "ng test".
8983
* It supports filtering by spec name if the underlying builder supports it (e.g., 'unit-test' builder).
84+
* This runs a headless Chrome as a browser, so requires Chrome to be installed.
9085
</Operational Notes>
9186
`,
9287
isReadOnly: false,

packages/angular/cli/src/commands/mcp/tools/test_spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,6 @@ describe('Test Tool', () => {
8282
const { structuredContent } = await runTest({ project: 'my-failed-app' }, mockHost);
8383

8484
expect(structuredContent.status).toBe('failure');
85-
expect(structuredContent.logs).toEqual(testLogs);
85+
expect(structuredContent.logs).toEqual([...testLogs, 'Test failed']);
8686
});
8787
});

packages/angular/cli/src/commands/mcp/utils.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313

1414
import { workspaces } from '@angular-devkit/core';
1515
import { dirname, join } from 'node:path';
16-
import { LocalWorkspaceHost } from './host';
16+
import { CommandError, LocalWorkspaceHost } from './host';
1717
import { McpToolContext } from './tools/tool-registry';
1818

1919
/**
@@ -95,3 +95,18 @@ export function getDefaultProjectName(context: McpToolContext): string | undefin
9595

9696
return undefined;
9797
}
98+
99+
/**
100+
* Get the logs of a failing command.
101+
*
102+
* This call has fallbacks in case the exception was thrown from the command-calling code itself.
103+
*/
104+
export function getCommandErrorLogs(e: unknown): string[] {
105+
if (e instanceof CommandError) {
106+
return [...e.logs, e.message];
107+
} else if (e instanceof Error) {
108+
return [e.message];
109+
} else {
110+
return [String(e)];
111+
}
112+
}
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.dev/license
7+
*/
8+
9+
import { join } from 'node:path';
10+
import { CommandError, LocalWorkspaceHost } from './host';
11+
import { addProjectToWorkspace, createMockContext } from './testing/test-utils';
12+
import {
13+
createStructuredContentOutput,
14+
findAngularJsonDir,
15+
getCommandErrorLogs,
16+
getDefaultProjectName,
17+
getProject,
18+
} from './utils';
19+
20+
describe('MCP Utils', () => {
21+
describe('createStructuredContentOutput', () => {
22+
it('should create valid structured content output', () => {
23+
const data = { foo: 'bar' };
24+
const output = createStructuredContentOutput(data);
25+
26+
expect(output.structuredContent).toEqual(data);
27+
expect(output.content).toEqual([{ type: 'text', text: JSON.stringify(data, null, 2) }]);
28+
});
29+
});
30+
31+
describe('findAngularJsonDir', () => {
32+
let mockHost: typeof LocalWorkspaceHost;
33+
34+
beforeEach(() => {
35+
mockHost = {
36+
existsSync: jasmine.createSpy('existsSync'),
37+
} as unknown as typeof LocalWorkspaceHost;
38+
});
39+
40+
it('should return dir if angular.json exists in it', () => {
41+
(mockHost.existsSync as jasmine.Spy).and.callFake(
42+
(path: string) => path === join('/app', 'angular.json'),
43+
);
44+
expect(findAngularJsonDir('/app', mockHost)).toBe('/app');
45+
});
46+
47+
it('should traverse up directory tree', () => {
48+
(mockHost.existsSync as jasmine.Spy).and.callFake(
49+
(path: string) => path === join('/app', 'angular.json'),
50+
);
51+
expect(findAngularJsonDir('/app/src/app', mockHost)).toBe('/app');
52+
});
53+
54+
it('should return null if not found', () => {
55+
(mockHost.existsSync as jasmine.Spy).and.returnValue(false);
56+
expect(findAngularJsonDir('/app', mockHost)).toBeNull();
57+
});
58+
});
59+
60+
describe('getProject', () => {
61+
it('should return undefined if workspace has no projects', () => {
62+
const { context } = createMockContext();
63+
const emptyContext = { ...context };
64+
expect(getProject(emptyContext, 'app')).toBeUndefined();
65+
});
66+
67+
it('should return undefined if project not found', () => {
68+
const { context, projects } = createMockContext();
69+
addProjectToWorkspace(projects, 'existing-app', {}, 'root');
70+
expect(getProject(context, 'non-existent')).toBeUndefined();
71+
});
72+
73+
it('should return project definition if found', () => {
74+
const { context, projects } = createMockContext();
75+
addProjectToWorkspace(projects, 'app', {}, 'root');
76+
77+
const project = getProject(context, 'app');
78+
expect(project).toBeDefined();
79+
expect(project?.root).toBe('root');
80+
});
81+
});
82+
83+
describe('getDefaultProjectName', () => {
84+
it('should return undefined if workspace is missing', () => {
85+
const { context } = createMockContext();
86+
const emptyContext = { ...context, workspace: undefined };
87+
expect(getDefaultProjectName(emptyContext)).toBeUndefined();
88+
});
89+
90+
it('should return defaultProject from extensions', () => {
91+
const { context, workspace } = createMockContext();
92+
workspace.extensions['defaultProject'] = 'my-app';
93+
expect(getDefaultProjectName(context)).toBe('my-app');
94+
});
95+
96+
it('should return single project name if only one exists and no defaultProject', () => {
97+
const { context, projects } = createMockContext();
98+
addProjectToWorkspace(projects, 'only-app', {}, '');
99+
expect(getDefaultProjectName(context)).toBe('only-app');
100+
});
101+
102+
it('should return undefined if multiple projects exist and no defaultProject', () => {
103+
const { context, projects } = createMockContext();
104+
addProjectToWorkspace(projects, 'app1', {}, '');
105+
addProjectToWorkspace(projects, 'app2', {}, '');
106+
expect(getDefaultProjectName(context)).toBeUndefined();
107+
});
108+
});
109+
110+
describe('getCommandErrorLogs', () => {
111+
it('should extract logs from CommandError', () => {
112+
const logs = ['log1', 'log2'];
113+
const err = new CommandError('failed', logs, 1);
114+
expect(getCommandErrorLogs(err)).toEqual([...logs, 'failed']);
115+
});
116+
117+
it('should extract message from Error', () => {
118+
const err = new Error('oops');
119+
expect(getCommandErrorLogs(err)).toEqual(['oops']);
120+
});
121+
122+
it('should stringify unknown error', () => {
123+
expect(getCommandErrorLogs('weird error')).toEqual(['weird error']);
124+
});
125+
});
126+
});

0 commit comments

Comments
 (0)