feat(@angular/cli): add "test" and "e2e" MCP tools, and some test cleanup#32066
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This adds new tools for running unit and end-to-end tests via the MCP server.
amishne added area: @angular/cli action: review 
…for default projects
dgp1130 left a comment
dgp1130 left a comment There was a problem hiding this comment.
Lots of random thoughts and code suggestions, but overall LGTM. Thanks for adding this!
| returncreateStructuredContentOutput({ | ||
| status: 'failure', | ||
| logs: [ | ||
| `No e2e target is defined for project '${projectName??'default'}'. Please setup e2e testing first.`, |
There was a problem hiding this comment.
Consider: We should either link to documentation or call out direct usage of the ng e2e command as a mechanism to configure these tests.
Also typo: I think "setup" is a noun, "set up" would be using it as a verb.
| try{ | ||
| logs=(awaithost.runCommand('ng',args)).logs; | ||
| }catch(e){ | ||
| status='failure'; | ||
| if(einstanceofCommandError){ | ||
| logs=e.logs; | ||
| }elseif(einstanceofError){ | ||
| logs=[e.message]; | ||
| }else{ | ||
| logs=[String(e)]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider: Can this be refactored to a shared primitive somewhere? Seems like it would overlap pretty heavily with other executions of the CLI in the MCP server.
| letlogs: string[]=[]; | ||
| try{ | ||
| logs=(awaithost.runCommand('ng',args)).logs; |
There was a problem hiding this comment.
Question: I probably should have asked this on an earlier PR (maybe I did and forgot), but how is ng resolved? Is that looking for a global installation on the $PATH? Is it resolving within the project's node_modules somehow? I'm wondering if we want to call process.argv[0] to reuse the existing Angular CLI binary rather than attempting to find it again in the environment?
| expect(mockHost.runCommand).not.toHaveBeenCalled(); | ||
| }); | ||
| it('should error if default project does not have e2e target and no project specified',async()=>{ |
There was a problem hiding this comment.
Suggestion: A test which executes ng e2e successfully with the default project. I only see the error case here.
| } | ||
| // This is ran by the agent so we want a non-watched, headless test. | ||
| args.push('--browsers','ChromeHeadless'); |
There was a problem hiding this comment.
Consider: What if someone doesn't have Chrome installed and instead uses Firefox or Safari?
@clydin is there a way to trigger "headless" without specifying a specific browser? The alternative I can think of is setting the CI=true environment variable.
There was a problem hiding this comment.
Consider: Add tests for these utilities directly. It can potentially reduce the need for some test coverage in the MCP tools as well.
| getAvailablePort: jasmine | ||
| .createSpy<Host['getAvailablePort']>('getAvailablePort') | ||
| .and.resolveTo(0), |
There was a problem hiding this comment.
Consider: Could we put the auto-incrementing port number implementation here and potentially reuse it across all affected tests?
| constmock=createMockContext(); | ||
| mockHost=mock.host; | ||
| mockContext=mock.context; | ||
| mockProjects=mock.projects; | ||
| mockWorkspace=mock.workspace; |
There was a problem hiding this comment.
Consider: If we can make this mock setup easier (potentially with the fake implementation proposed in a separate comment) we can move this out of the beforeEach and into each test, so each one can get a unique environment for its needs rather than attempting to share and then retroactively configure a base environment.
| * Creates a mock implementation of the Host interface for testing purposes. | ||
| * Each method is a Jasmine spy that can be configured. | ||
| */ | ||
| exportfunctioncreateMockHost(): MockHost{ |
There was a problem hiding this comment.
Consider: Is there a path to a "fake" host instead of a "mock" host? Could we provide fake implementations for stat and existsSync if we accept a virtual file system (or create a real one in a temp directly from inputs)?
Faking implementation of runCommand and spawn is likely a bit harder, but we could either delegate the command to an input function or actually run a real subprocess to get more accurate error behavior. I'm thinking something along the lines of:
import*asfsfrom'fs/promises';classFakeHostimplementsHost{privatenextPort=0;privateconstructor(privatereadonlycmdImpls: Record<string,(...args: string[])=>CommandResult>,privatereadonlyroot: string){}staticasyncfrom(cmdImpls: Record<string,(...args: string[])=>CommandResult>,fs: Record<string,string|Buffer>={}): Promise<FakeHost>{consttempDir=awaitfs.mkdtemp();for(const[path,content]ofObject.entries(fs)){awaitfs.writeFile(path,content);}returnnewFakeHost(cmdImpls,tempDir);}stat(){/* Check in `root` */}existsSync(){/* Check in `root` */}getAvailablePort(): number{returnthis.nextPort++;}runCommand(binary: string,args: string[]): /* ... */{returnthis.cmdImpls[binary](args);}spawn(){/* Similar to `runCommand`... */}awaitdispose(): Promise<void>{awaitfs.rmdir(this.root);}}I'm thinking this can avoid the need to define spies unique to each test and increase overall test confidence. If we can make this specific to each test as suggested in a separate comment, it can potentially remove the need to mutate the environment after the beforeEach within a test.
Downside is that we do need to explicitly call dispose to clean up the temp directory if we're using a real file system. If we can run these tests on Node 24, we can potentially get away with using and Symbol.dispose which would help.
it('does a thing',()=>{await using host=awaitFakeHost.from(...);awaitrunE2e(host, ...);// Host implicitly disposed at end of scope.});| it('should proceed if no workspace context is available (fallback)',async()=>{ | ||
| // If context.workspace is undefined, it should try to run ng e2e (which might fail or prompt, but tool runs it) | ||
| constnoWorkspaceContext={}asMcpToolContext; | ||
| awaitrunE2e({},mockHost,noWorkspaceContext); | ||
| expect(mockHost.runCommand).toHaveBeenCalledWith('ng',['e2e']); | ||
| }); |
There was a problem hiding this comment.
Question: This is the case where we run the MCP server outside of a specific Angular CLI project right? I wonder if we should just fail in that case rather than attempting to run an ng e2e command which will almost certainly fail.
Thoughts @clydin?
2 participants
In addition to the unit tests, the test tool was also tested manually installed into Gemini-CLI.