Skip to content

test(BA-4974): add component tests for session lifecycle management#9891

Merged
HyeockJinKim merged 9 commits into
mainfrom
BA-4974
Mar 19, 2026
Merged

test(BA-4974): add component tests for session lifecycle management#9891
HyeockJinKim merged 9 commits into
mainfrom
BA-4974

Conversation

@jopemachine
Copy link
Copy Markdown
Member

@jopemachine jopemachine commented Mar 11, 2026

Resolves #9832 (BA-4974)

Summary

  • Add component tests for session restart, status transitions, and status history in test_session_lifecycle.py (20 tests)
  • Add component tests for session rename edge cases, destroy edge cases, and role-based permissions in test_session.py (10 tests added to existing file)
  • Add shared user_session_seed and scheduling_controller_mock fixtures to conftest.py

Test Distribution

test_session_lifecycle.py — Lifecycle & Status Transitions (20 tests)

Class Tests Description
TestSessionRestart 3 Restart running/terminated/nonexistent sessions
TestSessionStatusTransition 9 Single/batch transit, transition reflection, terminal state no-op, deregister filtering, role-based filtering, degraded session recovery & termination
TestSessionStatusHistory 6 Running/terminated/degraded history, full lifecycle (PENDING→SCHEDULED→PREPARING→PULLING→CREATING→RUNNING), user access, cross-user denied

test_session.py — CRUD & Permissions (10 tests added)

Class Tests Added Description
TestSessionRename 3 Rename back-and-forth reversibility, same-name rejection, user renames own session
TestSessionDestroy 2 Destroy already-terminated session (idempotent), user destroys own session
TestSessionPermissions 5 User gets own info, user cannot access/destroy/rename admin session, admin cannot access user session

Test plan

  • pants lint tests/component/session/ passes
  • pants check tests/component/session/ passes
  • pants test tests/component/session/ passes (all 5 test files)

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings March 11, 2026 03:30
@github-actions github-actions Bot added the size:L 100~500 LoC label Mar 11, 2026
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

Adds a new component test module to expand coverage of session lifecycle behavior through the SDK v2 client and manager REST handlers.

Changes:

  • Introduces tests/component/session/test_session_lifecycle.py with additional session lifecycle tests (restart, status transit, rename, permissions, status history, destroy).
  • Adds a DB-seeded “regular user owned” session fixture to validate role/ownership constraints.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +137 to +176
class TestSessionRestart:
"""Test session restart lifecycle."""

async def test_restart_running_session(
self,
admin_registry: BackendAIClientRegistry,
session_seed: SessionSeedData,
) -> None:
await admin_registry.session.restart(
session_seed.session_name,
RestartSessionRequest(),
)
# After restart, session should still be accessible
result = await admin_registry.session.get_info(session_seed.session_name)
assert isinstance(result, GetSessionInfoResponse)

async def test_restart_terminated_session_fails(
self,
admin_registry: BackendAIClientRegistry,
terminated_session_seed: SessionSeedData,
) -> None:
# Terminated sessions are stale and cannot be found for restart
with pytest.raises((NotFoundError, BackendAPIError)):
await admin_registry.session.restart(
terminated_session_seed.session_name,
RestartSessionRequest(),
)

async def test_restart_nonexistent_session_returns_not_found(
self,
admin_registry: BackendAIClientRegistry,
) -> None:
with pytest.raises(NotFoundError):
await admin_registry.session.restart(
"nonexistent-session-xyz-99999",
RestartSessionRequest(),
)


class TestSessionStatusTransition:
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

PR description/test count appears out of sync with the file contents: this module defines 21 test_* functions (2 marked xfail), but the PR metadata says 17 tests (and the per-class table totals don’t match). Please update the PR description/test table (or adjust the tests) so reviewers and CI expectations stay aligned.

Copilot uses AI. Check for mistakes.
Comment on lines +423 to +428
"""Destroying an already-terminated session succeeds (forced destroy is idempotent)."""
result = await admin_registry.session.destroy(
terminated_session_seed.session_name,
DestroySessionRequest(forced=True),
)
assert result is not None
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

The assertions in this test are too weak to validate the intended behavior (idempotent forced destroy). Other session component tests assert the concrete response type/shape for destroy(). Consider asserting the returned value is a DestroySessionResponse (and/or checking expected fields) so regressions don’t slip through while still returning a non-None object.

Copilot uses AI. Check for mistakes.
Comment on lines +442 to +443
# Verify the session status changed
assert result is not None
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

The inline comment says “Verify the session status changed”, but the test only asserts the destroy call returned a non-None response. Either add an assertion that confirms the session is no longer accessible / has TERMINATED status (e.g., get_info raises NotFound or status_history behavior), or remove/adjust the comment to match what’s actually verified.

Suggested change
# Verify the session status changed
assert result is not None
# Verify the session status changed and is no longer accessible
assert result is not None
with pytest.raises(NotFoundError):
await user_registry.session.get_info(
user_session_seed.session_name,
)

Copilot uses AI. Check for mistakes.
@jopemachine jopemachine added this to the 26.3 milestone Mar 11, 2026
jopemachine added a commit that referenced this pull request Mar 11, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jopemachine added a commit that referenced this pull request Mar 11, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jopemachine jopemachine marked this pull request as draft March 11, 2026 04:44
jopemachine added a commit that referenced this pull request Mar 11, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jopemachine jopemachine force-pushed the BA-4974 branch 2 times, most recently from e5e0229 to f18c299 Compare March 12, 2026 08:52
@github-actions github-actions Bot added size:XL 500~ LoC area:docs Documentations comp:manager Related to Manager component comp:agent Related to Agent component comp:client Related to Client component comp:common Related to Common component comp:app-proxy Related to App Proxy component require:db-migration Automatically set when alembic migrations are added or updated and removed size:L 100~500 LoC labels Mar 13, 2026
jopemachine and others added 5 commits March 19, 2026 11:51
Add tests for session restart, status transitions, rename lifecycle,
role-based permissions, status history, and destroy edge cases.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added size:L 100~500 LoC and removed size:XL 500~ LoC labels Mar 19, 2026
@github-actions github-actions Bot added size:XL 500~ LoC and removed size:L 100~500 LoC labels Mar 19, 2026
@jopemachine jopemachine modified the milestones: 26.3, Backlog, 26.4 Mar 19, 2026
@jopemachine jopemachine requested a review from a team March 19, 2026 03:51
@jopemachine jopemachine marked this pull request as ready for review March 19, 2026 03:52
@HyeockJinKim HyeockJinKim merged commit 08f9924 into main Mar 19, 2026
30 checks passed
@HyeockJinKim HyeockJinKim deleted the BA-4974 branch March 19, 2026 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:docs Documentations comp:agent Related to Agent component comp:app-proxy Related to App Proxy component comp:client Related to Client component comp:common Related to Common component comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated size:XL 500~ LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add component tests: Session lifecycle management

3 participants