Skip to content

test: add e2e session/sandbox lifecycle tests#298

Open
JagjeevanAK wants to merge 4 commits intovolcano-sh:mainfrom
JagjeevanAK:jagjeevan/e2e-sandbox
Open

test: add e2e session/sandbox lifecycle tests#298
JagjeevanAK wants to merge 4 commits intovolcano-sh:mainfrom
JagjeevanAK:jagjeevan/e2e-sandbox

Conversation

@JagjeevanAK
Copy link
Copy Markdown

@JagjeevanAK JagjeevanAK commented Apr 26, 2026

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds test/e2e/session_lifecycle_test.go with three net-new lifecycle tests from issue #103 that were not previously covered by e2e_test.go. All tests live in the existing e2e package and reuse the shared helpers already present in e2e_test.go.

Also adds test/e2e/echo_agent_short_ttl.yaml — an AgentRuntime CR with sessionTimeout: 30s required by the existing TestAgentRuntimeSessionTTL (A3) test.

Tests added:

Function Scenario What it asserts
TestAgentRuntimeSessionCreationAndReuse A1 First call without x-agentcube-session-id auto-creates a session and returns the header; second call with that ID returns the same session ID and correct output
TestCodeInterpreterSessionAutoCreation B1 POST to Router without session ID → correct stdout, exit 0, and x-agentcube-session-id set in response header
TestCodeInterpreterStatefulSession B2 State written to the shared workspace in one call (via file) is readable in the next call of the same session

Scenarios already covered by existing tests (no duplication):

Scenario Existing test
A2 – missing runtime → HTTP 404 TestAgentRuntimeErrorHandling
A3 – idle session TTL smoke test TestAgentRuntimeSessionTTL
B3 – file-based Fibonacci workflow TestCodeInterpreterFileOperations / "download file"

Which issue(s) this PR fixes:
Fixes #103

Special notes for your reviewer:

  • B2 uses a file written to the sandbox workspace to simulate state persistence across commands (each command runs in a fresh sub-process).
  • echo_agent_short_ttl.yaml uses the same echo-server logic as echo_agent.yaml but with sessionTimeout: 30s; apply it before running TestAgentRuntimeSessionTTL.

Does this PR introduce a user-facing change?:

NONE

@volcano-sh-bot
Copy link
Copy Markdown
Contributor

Welcome @JagjeevanAK! It looks like this is your first PR to volcano-sh/agentcube 🎉

@JagjeevanAK JagjeevanAK changed the title test: add e2e session/sandbox lifecycle tests (issue #103) test: add e2e session/sandbox lifecycle tests Apr 26, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces end-to-end tests for AgentCube session and sandbox lifecycles, along with a supporting AgentRuntime configuration with a short TTL. The tests verify session auto-creation, reuse, 404 error handling for missing runtimes, and stateful code interpreter sessions. Review feedback suggests improving test robustness by checking ignored errors from io.ReadAll, using t.Fatal instead of t.Skip when session creation fails to properly signal a system failure, and reporting premature session invalidation as an error rather than a log message.

Comment thread test/e2e/session_lifecycle_test.go Outdated
Comment thread test/e2e/session_lifecycle_test.go Outdated
Comment thread test/e2e/session_lifecycle_test.go Outdated
@JagjeevanAK
Copy link
Copy Markdown
Author

@hzxuzhonghu can you check this PR ?

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 26, 2026

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.58%. Comparing base (57f6d84) to head (26d48de).
⚠️ Report is 70 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #298      +/-   ##
==========================================
+ Coverage   43.37%   47.58%   +4.21%     
==========================================
  Files          30       30              
  Lines        2610     2820     +210     
==========================================
+ Hits         1132     1342     +210     
+ Misses       1355     1338      -17     
- Partials      123      140      +17     
Flag Coverage Δ
unittests 47.58% <100.00%> (+4.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Jagjeevan Kashid <jagjeevandev97@gmail.com>
@JagjeevanAK JagjeevanAK force-pushed the jagjeevan/e2e-sandbox branch from 4114bae to a6663e9 Compare April 26, 2026 16:18
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Comment thread test/e2e/session_lifecycle_test.go Outdated
Comment thread test/e2e/echo_agent_short_ttl.yaml Outdated
Signed-off-by: Jagjeevan Kashid <jagjeevandev97@gmail.com>
@JagjeevanAK
Copy link
Copy Markdown
Author

can you check the recent updates ?

CC: @acsoto

@acsoto
Copy link
Copy Markdown
Member

acsoto commented Apr 27, 2026

can you check the recent updates ?

CC: @acsoto

Thank you. E2E Tests failed. Cloud you pls check?

Signed-off-by: Jagjeevan Kashid <jagjeevandev97@gmail.com>
Copilot AI review requested due to automatic review settings April 27, 2026 10:39
@volcano-sh-bot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign yaozengzeng for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Jagjeevan Kashid <jagjeevandev97@gmail.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Comment thread test/e2e/session_lifecycle_test.go
Comment thread test/e2e/session_lifecycle_test.go
Comment thread test/e2e/session_lifecycle_test.go
@JagjeevanAK
Copy link
Copy Markdown
Author

Hey man Sorry for previous stuff now I think all test should pass easily

CC: @acsoto

@acsoto
Copy link
Copy Markdown
Member

acsoto commented Apr 27, 2026

LGTM

@JagjeevanAK
Copy link
Copy Markdown
Author

Any update on this ?

CC: @YaoZengzeng and @tjucoder

// Auto-created sessions come directly from WorkloadManager response, which
// currently does not include kind. Default to Sandbox so Router signs JWT
// when forwarding to sandbox runtimes (e.g. PicoD).
Kind: types.SandboxKind,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This field is missing set, because we cannot know whether it is sandbox or sandboxclaim now.

Do you see any problem without it?


// invokeCodeInterpreterWithHeader is like invokeCodeInterpreter but also returns
// the x-agentcube-session-id response header so B1 can assert on it.
func (e *testEnv) invokeCodeInterpreterWithHeader(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

prefer not make it a method

// 1. Returns HTTP 200.
// 2. Produces the expected stdout (e.g. "2\n" for print(1+1)).
// 3. Sets the x-agentcube-session-id response header.
func TestCodeInterpreterSessionAutoCreation(t *testing.T) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

isn't this included in TestAgentRuntimeSessionCreationAndReuse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Umbrella issues for E2E

6 participants