test(BA-4901): add unit service + component tests for User, Auth, and Group#9703
test(BA-4901): add unit service + component tests for User, Auth, and Group#9703HyeockJinKim merged 12 commits intomainfrom
Conversation
… user service actions Add test_user_service.py with tests for 6 missing user service actions: - BulkCreateUserAction (all success, partial failure, empty) - GetUserAction (found, not found) - BulkModifyUserAction (all success, partial failure, empty) - SearchUsersAction (pagination, offset beyond total) - SearchUsersByDomainAction (domain-scoped, pagination) - SearchUsersByProjectAction (members, empty) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…erScope auth actions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e actions Add tests/unit/manager/services/group/test_group_service.py with 17 tests covering 6 group service actions: - SearchProjectsAction: pagination, offset beyond total, is_active filter - SearchProjectsByDomainAction: domain-scoped projects, non-existent domain - SearchProjectsByUserAction: user-membership projects, no membership - GetProjectAction: existing project, non-existent raises ProjectNotFound - UsagePerMonthAction: valid month, group_ids filter, invalid format, no sessions - UsagePerPeriodAction: valid range, end<=start, >100 days, invalid format Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add registry-quota tests (create, read, permission checks) - Add xfail tests for group CRUD and member management (REST /groups routes not yet implemented on server side) - Add full lifecycle integration test (xfail) - Document deviations in DEVIATION.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ftest The registry quota component tests were failing with 500 errors because ContainerRegistryService was created without a quota_service. Added an InMemoryQuotaService implementation to satisfy the dependency. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ions Add CreateUser, ModifyUser, DeleteUser, PurgeUser, BulkPurgeUser, AdminMonthStats, UserMonthStats tests to user service test file. Add CreateGroup, ModifyGroup, DeleteGroup, PurgeGroup tests to group service test file. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive unit service tests and component tests for User, Auth, and Group services as part of BA-4901. The tests cover 12 User actions, 2 Auth actions, 10 Group actions, and SDK-level Group integration tests.
Changes:
- Add unit tests for User service actions (create, bulk create, get, modify, bulk modify, delete, purge, bulk purge, search variants, and month stats)
- Add unit tests for Auth service scope resolution (access key and user scope) and Group service actions (create, modify, delete, purge, search variants, get, usage stats)
- Add component tests for SDK Group integration with
xfailmarkers for unimplemented REST routes, plus a deviation report documenting these gaps
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/manager/services/user/test_user_service.py | Unit tests for 12 UserService actions with mocked repository |
| tests/unit/manager/services/group/test_group_service.py | Unit tests for 10 GroupService actions with mocked repository |
| tests/unit/manager/services/group/BUILD | Pants build file for group unit tests |
| tests/unit/manager/services/auth/test_resolve_scope.py | Unit tests for 2 AuthService scope resolution actions |
| tests/component/group/test_group.py | Component tests for SDK Group integration (registry quota, CRUD, members, lifecycle) |
| tests/component/group/conftest.py | Test fixtures for component tests including in-memory quota service |
| tests/component/group/BUILD | Pants build file for group component tests |
| changes/9703.test.md | Changelog entry for the test additions |
| DEVIATION.md | Documents deviations from test plan due to unimplemented REST routes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def auth_service( | ||
| mock_hook_plugin_ctx: AsyncMock, | ||
| mock_auth_repository: AsyncMock, | ||
| mock_config_provider: AsyncMock, | ||
| ) -> AuthService: |
There was a problem hiding this comment.
The auth_service fixture depends on mock_hook_plugin_ctx and mock_config_provider fixtures, but these are not defined in this file or visible in the provided conftest.py. If these fixtures are not defined in a shared conftest, the tests will fail at collection time with a fixture not found error. Either define these fixtures locally in this file or ensure they exist in a parent conftest.
|
|
||
| import uuid | ||
| from datetime import UTC, datetime | ||
| from unittest.mock import AsyncMock, MagicMock, PropertyMock, patch |
There was a problem hiding this comment.
patch is imported but only used once (line 701) via @patch.object. Consider whether using it as a decorator import from unittest.mock is intentional. More importantly, IntegrityError from SQLAlchemy (line 15) is used as the expected exception in test_duplicate_name_raises_conflict, but typically the service layer should translate database-level exceptions into domain exceptions (like a custom conflict error). This test is asserting that a raw SQLAlchemy exception propagates through the service, which may indicate a missing error handling layer in the service.
| import pytest | ||
|
|
||
| from ai.backend.common.data.user.types import UserRole | ||
| from ai.backend.common.exception import InvalidAPIParameters |
There was a problem hiding this comment.
InvalidAPIParameters is imported but only used within BulkCreatorError construction on line 303. The Updater import on line 36 is used in TestModifyUser but BatchQuerier on line 35 is also used. However, the repeated fixture pattern of mock_user_repository + service is duplicated across every test class (9 times). Consider extracting these to module-level fixtures or using a base class to reduce boilerplate.
| domain_name=domain_name, | ||
| total_resource_slots=ResourceSlot.from_user_input({}, None), | ||
| allowed_vfolder_hosts=VFolderHostPermissionMap({}), | ||
| dotfiles=b"\x90", |
There was a problem hiding this comment.
The magic byte b\"\\x90\" for dotfiles is unexplained. If this represents a specific format (e.g., msgpack empty map), it would be clearer to add a brief comment or use a named constant to explain why this specific value was chosen for the test fixture.
| @pytest.fixture() | ||
| async def target_group( | ||
| db_engine: SAEngine, | ||
| domain_fixture: str, | ||
| resource_policy_fixture: str, | ||
| ) -> AsyncIterator[uuid.UUID]: |
There was a problem hiding this comment.
The target_group fixture uses db_engine: SAEngine (raw SQLAlchemy async engine), while container_registry_processors on line 49 uses database_engine: ExtendedAsyncSAEngine. These are likely different fixtures. If db_engine and database_engine don't point to the same database, the target_group row won't be visible to the container registry service, causing test failures. Verify these resolve to the same underlying database connection.
- Add type annotation `Any` to `**kwargs` in `_make_purge_action` - Use `cast(MagicMock, ...)` for `_agent_registry` method assignment - Add type hints to `mock_check_vfolder` closure - Use `Exception()` instead of `None` for `IntegrityError` orig param - Fix `read_quota` return type to match supertype `int` Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…onflict The abstract read_quota() returns int but the API response model supports int | None. Remove inheritance and cast at usage to let the test fixture return None for unset quotas without violating mypy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DEVIATION.md is a worker artifact that should not be committed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This reverts commit f082715.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Test plan
pants test tests/unit/manager/services/user::pants test tests/unit/manager/services/auth::pants test tests/unit/manager/services/group::pants test tests/component/group::pants lint --changed-since=origin/mainResolves BA-4901