Add the integration test to validate latest vs code with the elicitat…#2549
Add the integration test to validate latest vs code with the elicitat…#2549xiangyan99 wants to merge 9 commits intomainfrom
Conversation
…ion feature. Co-authored-by: Copilot <copilot@github.com>
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
A couple of issues spotted in this draft:
The path normalization in the new PS script uses double-backslash replacement (Replace('\\\\', '/')) but every other script in eng/scripts/ uses single-backslash (Replace('\\', '/')). On Windows, \.Path returns paths like C:\code\mcp, so the double-backslash version won't match anything and the path stays unconverted.
The outerloop-test npm script installs @playwright/test@latest at runtime with --no-save --no-package-lock. This means different CI runs can pick up different Playwright versions - a Playwright breaking change could cause failures unrelated to code changes. Consider pinning to a major version (e.g. @playwright/test@^1) or adding a comment explaining the intent behind using @latest.
Minor: the test creates a temp directory via fs.mkdtemp but doesn't clean it up in the finally block. On long-lived CI agents, these accumulate.
Also the PR description template isn't filled in - it'd help reviewers to know which issue this addresses and what the testing strategy looks like.
| . "$PSScriptRoot/../common/scripts/common.ps1" | ||
|
|
||
| $RepoRoot = $RepoRoot.Path.Replace('\\', '/') | ||
| $projectPath = Join-Path $RepoRoot $VsCodeProjectPath |
There was a problem hiding this comment.
This should be Replace('\', '/') (single backslash) to match the convention in every other script under eng/scripts/. The double-backslash version won't match standard Windows paths like C:\code\mcp.
| "lint": "eslint src --ext ts", | ||
| "test": "npm run compile && node ./out/test/runAllTests.js", | ||
| "unit-test": "npm run compile && node ./out/test/runUnitTests.js" | ||
| "unit-test": "npm run compile && node ./out/test/runUnitTests.js", |
There was a problem hiding this comment.
Installing @playwright/test@latest at runtime with no lockfile means different CI runs can get different versions. If the intent is to always test against the newest Playwright, a comment explaining that would help. Otherwise, pinning to a major version (@playwright/test@^1) gives reproducibility without blocking upgrades.
| await runCommand(window, 'MCP: Run Tool'); | ||
| await selectFromQuickInput(window, MCP_SERVER_NAME); | ||
| await selectFromQuickInput(window, TOOL_NAME); | ||
| await fillCurrentQuickInput(window, 'test-subscription'); |
There was a problem hiding this comment.
tempRoot isn't cleaned up in the finally block. Adding await fs.rm(tempRoot, { recursive: true, force: true }); alongside app.close() prevents temp directory accumulation on CI agents.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Two new commits since my last review. The test now installs a test-only VS Code extension that auto-invokes a sensitive MCP tool on startup, rather than driving the command palette manually. This removes timing dependencies on the quick input widget and is more reliable for CI.
Other improvements:
- Playwright tracing, screenshot, and HTML capture on failure for easier CI debugging
- Frame-aware polling for the elicitation text (needed because VS Code renders webviews in iframes)
- Pipeline artifact publishing via the 1ES template
My three prior comments are still open (backslash convention in PS script, Playwright version pinning, temp dir cleanup). No new issues in the incremental changes.
2 participants
Add the integration test to validate latest vs code with the elicitation feature.
What does this PR do?
[Provide a clear, concise description of the changes][Add additional context, screenshots, or information that helps reviewers]GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationREADME.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1. See Package READMEToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.jsonbreaking-changelabelservers/Azure.Mcp.Server/docs/azmcp-commands.md./eng/scripts/Update-AzCommandsMetadata.ps1to update tool metadata inazmcp-commands.md(required for CI)servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline