test: add browser tests for agent inspector#938
Conversation
|
/strands review |
Review SummaryThanks for adding browser tests for the agent inspector! This is a solid implementation that follows Playwright best practices and integrates well with the existing testing infrastructure. ✅ Strengths
💡 SuggestionsHigh Priority
Medium Priority
Low Priority
📝 Code-Specific Feedbackbrowser-tests/global-setup.ts (lines 76-86) // Consider wrapping the entire create command in a more descriptive try-catch
// that explains what went wrong and provides troubleshooting stepsplaywright.config.ts (line 19) retries: 0,
// Consider: retries: 1, // Browser tests can be flakyglobal-teardown.ts
🎯 Questions
Overall AssessmentThis is a high-quality PR that adds valuable test coverage for the agent inspector. The implementation is solid, follows best practices, and integrates well with the existing codebase. The suggestions above are mostly enhancements rather than blockers. Once you address the high-priority items (especially unit tests for Great work! 🎉 |
3ef2f64 to
f36b7b4
Compare
f36b7b4 to
a6164fa
Compare
|
/strands review |
Description
Adds a Playwright-based browser test suite for the agent inspector web UI served by
agentcore dev. The tests create a temporary Strands project, start the dev server, and exercise the inspector through a real Chromium browser.Test coverage:
Infrastructure changes:
agentcore create --json, startsagentcore devvia node-pty, waits for the web UI port, and cleans up afterwardtestEnv) provides port, project path, and project name to testsAGENT_INSPECTOR_PATHenv var support toresolveUIDistDir()so tests (and developers) can point at a local agent-inspector buildresolveUIDistDirexported for testability, with unit tests addede2e-tests-fullCI workflowbrowser-tests/(same treatment as other test dirs)tsconfig.jsonincludesbrowser-tests/Bug fix:
runningAgents.set()in the start handler to after thewaitForServerReadycheck passes. Previously,/api/statuscould report an agent as running before the agent was actually accepting connections, causing the frontend to send invocations that hit ECONNREFUSED. The readiness check itself remains TCP-based (createConnection) to avoid polluting agent telemetry with health-check HTTP spans.Type of Change
Testing
npm run test:unitandnpm run test:integnpm run typechecknpm run lintBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.