diff --git a/agents/test-plan-generator/hackbot_agents/test_plan_generator/prompts/system.md b/agents/test-plan-generator/hackbot_agents/test_plan_generator/prompts/system.md index bab8eee10f..c65e4db8d1 100644 --- a/agents/test-plan-generator/hackbot_agents/test_plan_generator/prompts/system.md +++ b/agents/test-plan-generator/hackbot_agents/test_plan_generator/prompts/system.md @@ -2,8 +2,8 @@ You are a Firefox QA test-plan generation and execution agent. Generate test cases from the provided Firefox feature name, feature description, and test scope, run them in Firefox with the available DevTools MCP tools, and -record the generated test plan for TestRail. Do not try to fix, patch or make -changes. +record the generated test plan for TestRail, each case carrying its `passed`, +`failed` or `unsuitable` result. Do not try to fix, patch or make changes. ## Required workflow @@ -12,9 +12,13 @@ changes. 2. Each test case must have: - A title. - Ordered test steps, each with an `action` and optional `expectation`. + - After execution, one nested `result` containing the case status, a concise + summary, and any failure reason. 3. Run the generated cases and steps in order. 4. Record one final TestRail action with `testrail_submit_test_plan`. - Use the provided feature name as the action feature. + - Include the execution result inside each generated test case. + - Set `summary` to a short overview of how the run went as a whole. ## Context guidance @@ -33,10 +37,10 @@ bypass a failing content interaction. - Do not skip, reorder, combine, or rewrite steps after generation. - Call only the tools needed for the current step. -- If a step fails, mark that step failed, mark the case failed, stop that case, - and move to the next case. -- When a step fails, include a concise failure reason based only on observed - behavior. +- If a step fails, mark the case failed, stop that case, and move to the next + case. +- When a step fails, name the step and include the observed behavior in the + case result summary. - When a case fails or is unsuitable, include a concise case-level reason. - Do not try alternate approaches to make a failing step pass. @@ -68,8 +72,11 @@ Mark a case as `unsuitable` only if it requires: ## Reporting -Record the generated test plan through `testrail_submit_test_plan` exactly once. -A prose message is not enough. +Record the generated test plan and execution outcomes through +`testrail_submit_test_plan` exactly once. A prose message is not enough. Include +one nested `result` for every generated test case. -Then close with a write-up of the execution: which cases passed, failed, or were -unsuitable, with concise observations for the failed and unsuitable ones. +Write the overall write-up once, in the action's `summary`: which cases passed, +failed, or were unsuitable, with concise observations for the failed and +unsuitable ones. It becomes the description of the TestRail run, so it is what a +QA engineer reads first. diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py b/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py index e429dd229c..72fe1d9286 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py @@ -2,7 +2,7 @@ from __future__ import annotations -from typing import Annotated, Any +from typing import Annotated, Any, Literal from agent_tools.registry import ToolError, tool, tools_in from pydantic import ( @@ -26,6 +26,23 @@ class TestRailStepInput(BaseModel): ) +class TestRailCaseResultInput(BaseModel): + status: Literal["passed", "failed", "unsuitable"] + summary: str + failure_reason: str | None = Field( + default=None, + description="Required when status is failed or unsuitable.", + ) + + @model_validator(mode="after") + def failure_reason_required_for_non_passing_cases( + self, + ) -> "TestRailCaseResultInput": + if self.status in {"failed", "unsuitable"} and not self.failure_reason: + raise ValueError("failed or unsuitable cases must include failure_reason") + return self + + class TestRailCaseInput(BaseModel): id: int title: str = Field(description="TestRail test case title.") @@ -39,6 +56,11 @@ class TestRailCaseInput(BaseModel): "and an optional expectation." ), ) + result: TestRailCaseResultInput = Field( + description=( + "Execution result for this generated test case after the agent ran it." + ) + ) @field_validator("title") @classmethod @@ -71,6 +93,10 @@ class SubmitTestPlanInput(BaseModel): max_length=30, description="Generated test cases to upload to TestRail.", ) + summary: str | None = Field( + default=None, + description="Optional summary of the generated test-plan execution.", + ) @field_validator("feature") @classmethod @@ -93,10 +119,19 @@ def _confirm(recorder: ActionsRecorder, action_type: str) -> str: return f"Recorded {action_type} (#{len(recorder.actions) - 1})." -def _validated_params(feature: str, generated_test_cases: list[Any]) -> dict[str, Any]: +def _validated_params( + feature: str, + generated_test_cases: list[Any], + *, + summary: str | None = None, +) -> dict[str, Any]: try: validated = SubmitTestPlanInput.model_validate( - {"feature": feature, "generated_test_cases": generated_test_cases} + { + "feature": feature, + "generated_test_cases": generated_test_cases, + "summary": summary, + } ) except ValidationError as exc: raise ToolError( @@ -128,6 +163,10 @@ async def submit_test_plan( description="Generated test cases to upload together to TestRail.", ), ], + summary: Annotated[ + str | None, + Field(description="Short overview of how the run went as a whole."), + ] = None, ) -> str: """Record a generated test plan for deferred TestRail submission. @@ -141,7 +180,11 @@ async def submit_test_plan( "a test plan is already recorded for this run; do not call " "submit_test_plan again" ) - params = _validated_params(feature, generated_test_cases) + params = _validated_params( + feature, + generated_test_cases, + summary=summary, + ) recorder.record(ACTION_TYPE, params) return _confirm(recorder, ACTION_TYPE) diff --git a/libs/hackbot-runtime/tests/test_testrail_action.py b/libs/hackbot-runtime/tests/test_testrail_action.py index 4c17ed077c..205bfaa803 100644 --- a/libs/hackbot-runtime/tests/test_testrail_action.py +++ b/libs/hackbot-runtime/tests/test_testrail_action.py @@ -15,6 +15,10 @@ def _cases(): "steps": [ {"action": "Open the PDF", "expectation": "The PDF is displayed."} ], + "result": { + "status": "passed", + "summary": "Worked.", + }, } ] @@ -38,8 +42,14 @@ async def test_submit_test_plan_tool_records_deferred_action(): "steps": [ {"action": "Open the PDF", "expectation": "The PDF is displayed."} ], + "result": { + "status": "passed", + "summary": "Worked.", + "failure_reason": None, + }, } ], + "summary": None, } @@ -55,6 +65,7 @@ async def test_submit_test_plan_tool_rejects_invalid_input(): "id": 1, "title": "Case", "steps": [], + "result": {"status": "passed", "summary": "Worked."}, } ], ) @@ -75,6 +86,7 @@ async def test_submit_test_plan_tool_rejects_cases_without_expectation(): "id": 1, "title": "Case", "steps": [{"action": "Open the PDF", "expectation": None}], + "result": {"status": "passed", "summary": "Worked."}, } ], ) @@ -144,6 +156,7 @@ async def test_submit_test_plan_tool_preserves_blank_expectations(): {"action": "Open the PDF", "expectation": ""}, {"action": "Select text", "expectation": "Text is selected."}, ], + "result": {"status": "passed", "summary": "Worked."}, } ], ) @@ -154,5 +167,55 @@ async def test_submit_test_plan_tool_preserves_blank_expectations(): ] +async def test_submit_test_plan_tool_records_execution_results(): + recorder = ActionsRecorder() + + await testrail.submit_test_plan( + recorder, + feature="Feature", + generated_test_cases=_cases(), + summary="All executable cases passed.", + ) + + assert recorder.actions[0]["params"]["generated_test_cases"][0]["result"] == { + "status": "passed", + "summary": "Worked.", + "failure_reason": None, + } + assert recorder.actions[0]["params"]["summary"] == "All executable cases passed." + + +async def test_submit_test_plan_tool_rejects_missing_case_result(): + recorder = ActionsRecorder() + cases = _cases() + del cases[0]["result"] + + with pytest.raises(ToolError) as exc: + await testrail.submit_test_plan( + recorder, + feature="Feature", + generated_test_cases=cases, + ) + + assert "invalid TestRail submission" in str(exc.value) + assert recorder.actions == [] + + +async def test_submit_test_plan_tool_rejects_not_run_results(): + recorder = ActionsRecorder() + cases = _cases() + cases[0]["result"] = {"status": "not_run", "summary": "Not run."} + + with pytest.raises(ToolError) as exc: + await testrail.submit_test_plan( + recorder, + feature="Feature", + generated_test_cases=cases, + ) + + assert "invalid TestRail submission" in str(exc.value) + assert recorder.actions == [] + + def test_submit_test_plan_handler_is_registered(): assert isinstance(get_handler(ACTION_TYPE), SubmitTestPlanHandler) diff --git a/libs/hackbot-runtime/tests/test_testrail_handler.py b/libs/hackbot-runtime/tests/test_testrail_handler.py index 9fae0d8f56..3640b65673 100644 --- a/libs/hackbot-runtime/tests/test_testrail_handler.py +++ b/libs/hackbot-runtime/tests/test_testrail_handler.py @@ -25,6 +25,11 @@ def _plan(): "expectation": "Text selection is highlighted in the PDF.", }, ], + "result": { + "status": "passed", + "summary": "The PDF behaved as expected.", + "failure_reason": None, + }, }, { "id": 2, @@ -36,42 +41,11 @@ def _plan(): "expectation": "The toolbar remains visible and usable.", }, ], - }, - ], - "results": [ - { - "id": 1, - "status": "passed", - "summary": "The PDF behaved as expected.", - "failure_reason": None, - "step_results": [ - { - "step_number": 1, - "status": "passed", - "observation": "The PDF opened.", - "failure_reason": None, - }, - { - "step_number": 2, - "status": "passed", - "observation": "Text was selected.", - "failure_reason": None, - }, - ], - }, - { - "id": 2, - "status": "unsuitable", - "summary": "The toolbar could not be inspected.", - "failure_reason": "No available tool can inspect it.", - "step_results": [ - { - "step_number": 1, - "status": "not_run", - "observation": "Not run.", - "failure_reason": None, - } - ], + "result": { + "status": "unsuitable", + "summary": "The toolbar could not be inspected.", + "failure_reason": "No available tool can inspect it.", + }, }, ], "summary": "One passed and one was unsuitable.", diff --git a/services/hackbot-ui/components/TestPlanView.tsx b/services/hackbot-ui/components/TestPlanView.tsx index 044a622dec..2d5d5ba41f 100644 --- a/services/hackbot-ui/components/TestPlanView.tsx +++ b/services/hackbot-ui/components/TestPlanView.tsx @@ -8,6 +8,7 @@ interface GeneratedTestCase { title: string; preconditions: string | null; steps: TestStep[]; + result: TestCaseResult | null; } interface TestStep { @@ -25,7 +26,6 @@ interface TestCaseResult { export interface TestPlan { feature: string; generatedTestCases: GeneratedTestCase[]; - results: TestCaseResult[]; summary: string; } @@ -53,12 +53,23 @@ function isTestStep(value: TestStep | null): value is TestStep { return value !== null; } +function parseCaseResult(value: unknown, id: number): TestCaseResult | null { + if (!isPlainObject(value) || !isStatus(value.status)) { + return null; + } + return { + id, + status: value.status, + summary: typeof value.summary === "string" ? value.summary : "", + failureReason: + typeof value.failure_reason === "string" ? value.failure_reason : null, + }; +} + export function parseTestPlan( findings: Record, actions: RunAction[] | null = null ): TestPlan | null { - // The agent now records the plan straight onto the action, so that is the - // source of truth. Runs from before the switch still carry it in findings. const testRailAction = actions?.find( (action) => action.type === "testrail.submit_test_plan" && @@ -70,6 +81,18 @@ export function parseTestPlan( return null; } + const legacyResultsById = new Map(); + if (Array.isArray(result.results)) { + for (const value of result.results) { + if (isPlainObject(value) && typeof value.id === "number") { + const parsed = parseCaseResult(value, value.id); + if (parsed) { + legacyResultsById.set(value.id, parsed); + } + } + } + } + const generatedTestCases: GeneratedTestCase[] = []; for (const value of result.generated_test_cases) { if ( @@ -90,43 +113,21 @@ export function parseTestPlan( preconditions: typeof value.preconditions === "string" ? value.preconditions : null, steps, + result: + parseCaseResult(value.result, value.id) ?? + legacyResultsById.get(value.id) ?? + null, }); } - const results: TestCaseResult[] = []; - if (Array.isArray(result.results)) { - for (const value of result.results) { - if ( - isPlainObject(value) && - typeof value.id === "number" && - isStatus(value.status) - ) { - results.push({ - id: value.id, - status: value.status, - summary: typeof value.summary === "string" ? value.summary : "", - failureReason: - typeof value.failure_reason === "string" - ? value.failure_reason - : null, - }); - } - } - } - return { feature: typeof result.feature === "string" ? result.feature : "", generatedTestCases, - results, summary: typeof result.summary === "string" ? result.summary : "", }; } export function TestPlanView({ testPlan }: { testPlan: TestPlan }) { - const resultsById = new Map( - testPlan.results.map((result) => [result.id, result]) - ); - return (

Test plan

@@ -139,7 +140,7 @@ export function TestPlanView({ testPlan }: { testPlan: TestPlan }) {

Test cases

    {testPlan.generatedTestCases.map((testCase) => { - const result = resultsById.get(testCase.id); + const result = testCase.result; const failureReason = result && result.status !== "passed" ? result.failureReason || result.summary