feat: add integration deployment test for LangGraph HITL agent#90
Merged
mpk-droid merged 3 commits intoMay 6, 2026
Merged
Conversation
Add test_deployment.py for the human_in_the_loop agent following the same pattern as react_agent: build-openshift, deploy via Helm, validate GET /health returns 200, and teardown via fixture finalizer. Also introduces load_agent_name() in shared utils to read the agent name from agent.yaml instead of hardcoding it (addresses PR #83 feedback), and updates react_agent's test to use it. Jira: RHAIENG-4644 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughCI test matrix adds ChangesDeployment Test & Integration Flow
Sequence DiagramsequenceDiagram
participant CI as "GitHub Actions (agent-deployment-test)"
participant Make as "Agent Makefile"
participant Registry as "Internal Image Registry"
participant Cluster as "OpenShift Cluster"
participant Test as "pytest (integration test)"
CI->>Make: run `make build-openshift`
Make->>Registry: build image & push to internal registry
CI->>Make: run `make deploy`
Make->>Cluster: apply manifests / create deployment & route
Test->>Cluster: poll GET {route}/health (retry/backoff)
Cluster-->>Test: returns JSON { status, agent_initialized }
Test-->>CI: upload `results.xml` artifact
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/integration/utils.py (1)
15-20: ⚡ Quick winUse
yaml.safe_load()instead of regex to parseagent.yamlThe regex
r"^name:\s*(.+)"won't strip inline YAML comments (name: my-agent # description→ capturesmy-agent # description) or unquote quoted values (name: "my-agent"→ captures"my-agent"), which would corrupt the image tag and route name used downstream.♻️ Proposed refactor
+import yaml + def load_agent_name(agent_dir: str | Path) -> str: - text = (Path(agent_dir) / "agent.yaml").read_text() - match = re.search(r"^name:\s*(.+)", text, re.MULTILINE) - if not match: + data = yaml.safe_load((Path(agent_dir) / "agent.yaml").read_text()) + if not isinstance(data, dict) or "name" not in data: raise ValueError(f"No 'name' field in {agent_dir}/agent.yaml") - return match.group(1).strip() + return str(data["name"]).strip()Note: the Makefile
AGENT_NAMEshell snippet uses the same regex, so if you update the Python helper you should align the Makefile too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/utils.py` around lines 15 - 20, The load_agent_name function currently uses a regex that captures inline comments and quoted strings; replace it with YAML parsing: read the agent.yaml, parse with yaml.safe_load (import yaml), extract the 'name' key, validate it's present and a string, and return its stripped value; raise a ValueError if missing or not a string. Update the corresponding Makefile AGENT_NAME shell snippet to use a YAML-aware extraction approach so both places remain consistent.agents/langgraph/human_in_the_loop/tests/integration/test_deployment.py (1)
31-90: ⚡ Quick win
_write_env_file,deployed_agent, andtest_health_endpointare verbatim copies of thereact_agentequivalentsThe only distinguishing fixture is
agent_dir. Everything else —INTERNAL_REGISTRY, env-file writing, build/deploy/undeploy flow, health-check assertion — is identical. When the HITL agent gains HITL-specific env requirements (e.g., a checkpointer DB URL) or the health response schema changes, one file will drift from the other.The shared pieces belong in
tests/integration/utils.py(or a sharedconftest.py), withagent_dirinjected as a parameter. The per-agent test module then only needs to defineagent_dirand any agent-specific assertions.♻️ Sketch of extraction (illustrative)
# tests/integration/utils.py (additions) +def write_env_file(agent_dir, container_image): + """Write a .env file so Makefile targets can source it.""" + missing = [v for v in ("BASE_URL", "MODEL_ID") if v not in os.environ] + if missing: + pytest.fail( + f"Missing required env vars: {', '.join(missing)}. " + "Set them in the CI workflow or export locally." + ) + env_path = agent_dir / ".env" + env_path.write_text( + f"API_KEY={os.environ.get('API_KEY', 'not-needed')}\n" + f"BASE_URL={os.environ['BASE_URL']}\n" + f"MODEL_ID={os.environ['MODEL_ID']}\n" + f"CONTAINER_IMAGE={container_image}\n" + ) + return env_path# tests/integration/conftest.py (shared fixture) +INTERNAL_REGISTRY = "image-registry.openshift-image-registry.svc:5000" + +@pytest.fixture(scope="module") +def deployed_agent(cluster_auth, agent_dir, agent_name): + ... # single shared implementation# agents/langgraph/human_in_the_loop/tests/integration/test_deployment.py -INTERNAL_REGISTRY = "image-registry.openshift-image-registry.svc:5000" - -def _write_env_file(...): ... - -@pytest.fixture(scope="module") -def deployed_agent(...): ... `@pytest.fixture`(scope="module") def agent_dir(repo_root): return repo_root / "agents" / "langgraph" / "human_in_the_loop" +# agent_name and deployed_agent come from shared conftest `@pytest.mark.integration` def test_health_endpoint(deployed_agent): ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agents/langgraph/human_in_the_loop/tests/integration/test_deployment.py` around lines 31 - 90, The module duplicates shared test logic (_write_env_file, deployed_agent fixture, and test_health_endpoint's health-check flow using INTERNAL_REGISTRY); extract these shared helpers into a common tests/integration/utils.py (or conftest.py) as functions/fixtures that accept agent_dir (e.g., write_env_file(agent_dir, container_image), deployed_agent(agent_dir, agent_name) or a parametrized fixture) and expose a reusable health_check helper, then update this file to only provide the agent-specific agent_dir fixture and any agent-specific assertions; ensure references to symbols _write_env_file, deployed_agent, INTERNAL_REGISTRY, and test_health_endpoint are replaced with the new shared imports and that imports/fixtures are adjusted accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@agents/langgraph/human_in_the_loop/tests/integration/test_deployment.py`:
- Around line 31-90: The module duplicates shared test logic (_write_env_file,
deployed_agent fixture, and test_health_endpoint's health-check flow using
INTERNAL_REGISTRY); extract these shared helpers into a common
tests/integration/utils.py (or conftest.py) as functions/fixtures that accept
agent_dir (e.g., write_env_file(agent_dir, container_image),
deployed_agent(agent_dir, agent_name) or a parametrized fixture) and expose a
reusable health_check helper, then update this file to only provide the
agent-specific agent_dir fixture and any agent-specific assertions; ensure
references to symbols _write_env_file, deployed_agent, INTERNAL_REGISTRY, and
test_health_endpoint are replaced with the new shared imports and that
imports/fixtures are adjusted accordingly.
In `@tests/integration/utils.py`:
- Around line 15-20: The load_agent_name function currently uses a regex that
captures inline comments and quoted strings; replace it with YAML parsing: read
the agent.yaml, parse with yaml.safe_load (import yaml), extract the 'name' key,
validate it's present and a string, and return its stripped value; raise a
ValueError if missing or not a string. Update the corresponding Makefile
AGENT_NAME shell snippet to use a YAML-aware extraction approach so both places
remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 1926fe1f-d92c-4d09-a342-db0688f2e911
📒 Files selected for processing (7)
.github/workflows/agent-deployment-test.yamlagents/langgraph/human_in_the_loop/Makefileagents/langgraph/human_in_the_loop/tests/integration/__init__.pyagents/langgraph/human_in_the_loop/tests/integration/conftest.pyagents/langgraph/human_in_the_loop/tests/integration/test_deployment.pyagents/langgraph/react_agent/tests/integration/test_deployment.pytests/integration/utils.py
tarun-etikala
approved these changes
May 6, 2026
Contributor
tarun-etikala
left a comment
tarun-etikala
left a comment
There was a problem hiding this comment.
Looks good Overall. Added minor comments
Addresses review feedback — regex could capture inline YAML comments or quoted values, silently corrupting image tags and route names. PyYAML is already a transitive dependency via LangGraph/LangChain. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/utils.py`:
- Around line 17-20: The code currently coerces any YAML value to a string and
returns it, which allows invalid values like null to become "None"; update the
logic that reads data["name"] to validate that the field exists, is an instance
of str, and that stripped value is non-empty before returning it (e.g., get the
raw value from data["name"], check isinstance(value, str), let name =
value.strip(), and if not name raise ValueError with a clear message); replace
the current str(data["name"]).strip() return with this validation and raise
behavior so only a non-empty string is returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: a6627203-55fd-4cc0-942b-0a1a132a4caf
📒 Files selected for processing (1)
tests/integration/utils.py
Comment on lines
+17
to
+20
| data = yaml.safe_load((Path(agent_dir) / "agent.yaml").read_text()) | ||
| if not isinstance(data, dict) or "name" not in data: | ||
| raise ValueError(f"No 'name' field in {agent_dir}/agent.yaml") | ||
| return str(data["name"]).strip() |
Contributor
There was a problem hiding this comment.
Validate name type/content before returning it
Line 20 currently coerces any YAML value to string (str(data["name"])), so invalid values like null become "None" and can propagate into deployment/route names. Fail fast by requiring a non-empty string.
Suggested fix
def load_agent_name(agent_dir: str | Path) -> str:
data = yaml.safe_load((Path(agent_dir) / "agent.yaml").read_text())
if not isinstance(data, dict) or "name" not in data:
raise ValueError(f"No 'name' field in {agent_dir}/agent.yaml")
- return str(data["name"]).strip()
+ name = data["name"]
+ if not isinstance(name, str) or not name.strip():
+ raise ValueError(f"Invalid 'name' value in {agent_dir}/agent.yaml: expected non-empty string")
+ return name.strip()As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| data = yaml.safe_load((Path(agent_dir) / "agent.yaml").read_text()) | |
| if not isinstance(data, dict) or "name" not in data: | |
| raise ValueError(f"No 'name' field in {agent_dir}/agent.yaml") | |
| return str(data["name"]).strip() | |
| def load_agent_name(agent_dir: str | Path) -> str: | |
| data = yaml.safe_load((Path(agent_dir) / "agent.yaml").read_text()) | |
| if not isinstance(data, dict) or "name" not in data: | |
| raise ValueError(f"No 'name' field in {agent_dir}/agent.yaml") | |
| name = data["name"] | |
| if not isinstance(name, str) or not name.strip(): | |
| raise ValueError(f"Invalid 'name' value in {agent_dir}/agent.yaml: expected non-empty string") | |
| return name.strip() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/utils.py` around lines 17 - 20, The code currently coerces
any YAML value to a string and returns it, which allows invalid values like null
to become "None"; update the logic that reads data["name"] to validate that the
field exists, is an instance of str, and that stripped value is non-empty before
returning it (e.g., get the raw value from data["name"], check isinstance(value,
str), let name = value.strip(), and if not name raise ValueError with a clear
message); replace the current str(data["name"]).strip() return with this
validation and raise behavior so only a non-empty string is returned.
Contributor
|
LGTM |
The `set -o pipefail` combined with `oc get all | head -10` causes exit code 141 when head closes the pipe before oc finishes writing. Add `|| true` since this step is diagnostic only. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
agents/langgraph/human_in_the_loop/) — builds on-cluster viabuild-openshift, deploys to OpenShift, validatesGET /healthreturns{"status": "healthy", "agent_initialized": true}, and tears down via fixture finalizerload_agent_name()shared utility intests/integration/utils.pythat reads the agent name fromagent.yamlinstead of hardcoding it (addresses PR #83 feedback from @andrewdonheiser)react_agentintegration test to useload_agent_name()as wellmake test-integrationtarget to HITL Makefile and adds the agent to the CI matrixTest plan
make teststill runs only unit tests (integration tests excluded via--ignore)make test-integrationruns the deployment test end-to-end on the demo clusterresults.xmlJira: RHAIENG-4644
🤖 Generated with Claude Code