Feat: Introduce targets to verify A2A and MCP servers start#209
Feat: Introduce targets to verify A2A and MCP servers start#209esnible wants to merge 12 commits intokagenti:mainfrom
Conversation
Signed-off-by: Ed Snible <snible@us.ibm.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
…lative path Signed-off-by: Ed Snible <snible@us.ibm.com>
…rent relative path Signed-off-by: Ed Snible <snible@us.ibm.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
mrsabath
left a comment
mrsabath
left a comment
There was a problem hiding this comment.
Review Summary
Well-structured PR that adds startup verification for all A2A and MCP example agents using Expect scripts, with a new test-startup-all Makefile target and CI job. This is valuable CI coverage that will catch dependency/import breakage early.
Areas reviewed: Makefile, CI/GitHub Actions, Expect scripts (.exp), .dockerignore
Commits: 12 commits, all signed-off ✓
CI status: All 10 checks passing ✓
Actions: All pinned to SHA ✓
A few minor suggestions below, nothing blocking. Nice work! 👍
| pushd $${f} || exit; \ | ||
| if [ -f test_startup.exp ]; then \ | ||
| echo "Testing startup for $${f}..."; \ | ||
| expect -f test_startup.exp || exit; \ |
There was a problem hiding this comment.
suggestion: test-startup-a2a and test-startup-mcp are nearly identical — the only difference is the top-level directory (a2a vs mcp). Could collapse into a shared pattern to reduce duplication, but not blocking since it's only ~10 lines each.
| exec pkill -P $child_pid | ||
| puts "Success"; exit 0 | ||
| } | ||
| timeout { |
There was a problem hiding this comment.
suggestion: The pkill -P $child_pid cleanup in the success branch is good. In the timeout branch you use kill -9 directly on the parent — if the child also spawns its own children, those grandchildren may survive. Consider using pkill -P in the timeout branch too for consistency.
| .git | ||
|
|
||
| # `expect` scripts | ||
| test_startup.exp No newline at end of file |
There was a problem hiding this comment.
nit: Missing trailing newline at end of file.
| run: pip install pytest pydantic pydantic-settings httpx python-dotenv | ||
| - name: Run tests | ||
| run: python -m pytest -v | ||
|
|
There was a problem hiding this comment.
praise: Good use of SHA-pinned actions with version comments. The timeout-minutes: 15 is a sensible guardrail.
2 participants
Summary
Resolves #207. Introduces a make target that verifies (most) examples that contain a test_startup.exp file are able to start. Introduces CI to run that target when checking PRs.
Every agent-example containing a test_startup.exp file, in Expect syntax checks to make sure the server completes enough work to start. If new examples are added, the authors should introduce a script.
I did it with a script for each example rather than a table because there were too many special cases.
Related issue(s)
#207
(Optional) Testing Instructions
To test,
make test-startup-allon your laptop before pushing a PR.Fixes #
#207