Skip to content

test(e2e): harden chat checks and CI gating - #119

Merged
rominf merged 2 commits into
mainfrom
test/e2e-follow-up-pr69
Jul 17, 2026
Merged

test(e2e): harden chat checks and CI gating#119
rominf merged 2 commits into
mainfrom
test/e2e-follow-up-pr69

Conversation

@rominf

@rominf rominf commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • require chat responses to contain a non-empty choices array
  • apply the existing inference timeout to tool-calling requests
  • run heavy CI when expectations.toml changes

Follow-up to #69, now rebased onto main.

Risk: low — this only tightens E2E assertions and CI selection.

Test plan

  • cargo fmt --all -- --check
  • cargo clippy -p e2e-cucumber --all-targets -- -D warnings
  • cargo test -p e2e-cucumber --lib
  • pre-push hooks: cargo clippy, cargo test, YAML and hygiene checks

@rominf
rominf requested a review from fredespi July 15, 2026 15:48
Base automatically changed from test/add-e2e-robot-framework to main July 15, 2026 15:49
@rominf
rominf force-pushed the test/e2e-follow-up-pr69 branch from a11f4cb to dec0f4b Compare July 15, 2026 16:18
@rominf
rominf marked this pull request as ready for review July 15, 2026 16:24
Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf
rominf force-pushed the test/e2e-follow-up-pr69 branch from dec0f4b to 6b269f0 Compare July 17, 2026 09:31
@rominf
rominf requested a review from volen-silo July 17, 2026 10:50

@volen-silo volen-silo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against the surrounding harness and ran the new unit test (cargo test -p e2e-cucumber --lib → 24 passed). All three changes are correct, focused, and low-risk. LGTM with one minor, non-blocking nit.

✅ Looks good

  • ci.yml — adding tests/e2e-cucumber/expectations.toml to the heavy filter is right: the e2e lanes gate on needs.changes.outputs.heavy == 'true', and an xfail-matrix edit should re-run them to validate the new expectations. Placed alongside **/*.feature, its natural sibling. (The other two files are .rs/already covered by heavy, so this closes the expectations-only-change gap.)
  • chat_response_is_successful + unit test — clean helper; the test covers the meaningful cases (missing key, null, non-array, empty array, non-empty array). Tightening assert_chat_successful from choices.is_some() to "non-empty choices array" is strictly better and won't false-negative a genuine completion (a valid OpenAI-style response always has a non-empty choices).
  • timeout on the tool-calling POST — mirrors send_chat's inference_timeout_for(world), so a hung tool request fails fast instead of blocking to the job cap. crate::inference_timeout_for resolves fine from the submodule.

🟡 Minor (non-blocking): the models-discovery GET in the tool path is still un-timed

In send_chat_with_tools, only the chat POST got the timeout client — the preceding reqwest::get(&models_url) (chat_steps.rs:45) is still the bare, timeout-less call. Its sibling send_chat (e2e.rs:543–556) runs both the models GET and the chat POST through the timeout client. So a stalled /v1/models could still hang the harness here, and the PR's "apply the timeout to tool-calling requests" goal is only partially met.

Low impact in practice (serve readiness is awaited separately, and /v1/models returns immediately once ready), but for consistency with send_chat consider routing the models GET through the same client:

let client = reqwest::Client::builder()
    .timeout(std::time::Duration::from_secs(crate::inference_timeout_for(world)))
    .build()
    .expect("failed to build HTTP client");
let resp: serde_json::Value = client.get(&models_url).send().await? ... ;
// reuse `client` for the POST below

Both hardened scenarios (4, 5) are @requires-gpu, so this only affects the GPU lanes, not the mock gate.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf

rominf commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review nit in 9702e95: the tool-calling path now constructs the timeout-configured client before model discovery and reuses it for both the /models GET and chat POST. Verified with formatting, targeted clippy/unit tests, and the full pre-push hooks.

@rominf
rominf enabled auto-merge July 17, 2026 11:33
@rominf
rominf added this pull request to the merge queue Jul 17, 2026
Merged via the queue into main with commit af88fc5 Jul 17, 2026
18 of 19 checks passed
@rominf
rominf deleted the test/e2e-follow-up-pr69 branch July 17, 2026 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants