test(BA-4900): add unit service + component tests for Image, ContainerRegistry, Export, ScalingGroup, and ErrorLog#9699
Conversation
Add tests/unit/manager/services/export/test_export_service.py with 17 tests covering all 7 Export service actions: - ListReportsAction: returns all reports, returns empty list - GetReportAction: valid key returns ReportDef, non-existent key raises ExportReportNotFound - ExportUsersCSVAction: encoding/fields/filename, custom filename, auto-generated timestamp filename, empty result headers only, large data streaming - ExportSessionsCSVAction: generated filename/fields, custom filename - ExportProjectsCSVAction: data accuracy, empty returns headers only - ExportKeypairsCSVAction: field export, auto-generated filename - ExportAuditLogsCSVAction: field export, auto-generated filename Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…calingGroup, ErrorLog Image: PreloadImage (NotImplementedError), ScanImage (success/errors/not-found), GetImageInstalledAgents (single/empty/non-existent) ContainerRegistry: HandleHarborWebhook (PUSH_ARTIFACT scan, auth validation, non-PUSH ignored, registry not found, multiple resources) ScalingGroup: GetWsproxyVersion (success, non-allowed group, no wsproxy addr, no client pool), ListAllowedScalingGroups (admin all, non-admin public, empty) ErrorLog: MarkClearedErrorLog (success, rowcount!=1, idempotent, cross-domain) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d Export CSV validation - ScalingGroup CRUD: multiple sgroups listed, domain association, public/private visibility, wsproxy version not configured, empty group association - Export CSV: UTF-8 encoding, field header validation, row content verification, secret_key exclusion, permission denied for regular users Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…er format CSV headers use human-readable names (e.g., "UUID", "Access Key") rather than field keys. Updated assertions to validate header count and structure instead of exact field name matches. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
Adds new unit and component test coverage for multiple manager services (Export, Image, ContainerRegistry, ScalingGroup, ErrorLog) to validate action behavior and key API flows (CRUD, CSV export, webhook handling).
Changes:
- Added unit service tests for Export/Image/ContainerRegistry/ScalingGroup/ErrorLog actions.
- Added component tests for ScalingGroup listing/visibility and Export CSV download/content checks.
- Added Pants BUILD target and changelog entry for the new tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/manager/services/scaling_group/test_scaling_group_service.py | Adds unit tests for wsproxy version lookup and listing allowed scaling groups |
| tests/unit/manager/services/image/test_image_service.py | Adds unit tests for preload, scan, and installed-agent lookup actions |
| tests/unit/manager/services/export/test_export_service.py | Introduces comprehensive unit tests for export report listing/getting and CSV export actions |
| tests/unit/manager/services/export/BUILD | Adds a Pants python_tests target for the new export unit tests |
| tests/unit/manager/services/error_log/test_error_log_service.py | Adds unit tests for marking error logs as cleared |
| tests/unit/manager/services/container_registry/test_container_registry_service.py | Adds unit tests for Harbor webhook handling scenarios |
| tests/component/scaling_group/test_scaling_group_crud.py | Adds component tests for scaling group visibility across user roles |
| tests/component/export/test_export_csv.py | Adds component tests validating CSV download and basic header/permission properties |
| changes/9699.test.md | Adds changelog note for the new tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def empty_iter() -> AsyncIterator[Sequence[Sequence[Any]]]: | ||
| return | ||
| yield # make this function an async generator |
There was a problem hiding this comment.
The yield statements are unreachable due to the preceding return, which makes these async generators rely on dead code for type/shape. Prefer a pattern that produces an empty async generator without unreachable lines (e.g., conditional if False: yield ...), so the intent is clear and static analyzers/coverage tools don’t flag this.
| async def empty_iter() -> AsyncIterator[Sequence[Sequence[Any]]]: | ||
| return | ||
| yield # make this function an async generator |
There was a problem hiding this comment.
The yield statements are unreachable due to the preceding return, which makes these async generators rely on dead code for type/shape. Prefer a pattern that produces an empty async generator without unreachable lines (e.g., conditional if False: yield ...), so the intent is clear and static analyzers/coverage tools don’t flag this.
| text = raw.decode("utf-8-sig") | ||
| reader = csv.reader(io.StringIO(text)) | ||
| headers = next(reader) | ||
| assert len(headers) == len(requested_fields) |
There was a problem hiding this comment.
These tests claim to validate that CSV headers “match the requested fields”, but they only assert the header count. This can pass even if the server returns the wrong fields or wrong ordering. Strengthen the assertions to compare headers to requested_fields (and, if ordering isn’t guaranteed by the API, assert set equality plus a separate ordering rule).
| assert len(headers) == len(requested_fields) | |
| assert headers == requested_fields |
| text = raw.decode("utf-8-sig") | ||
| reader = csv.reader(io.StringIO(text)) | ||
| headers = next(reader) | ||
| assert len(headers) == len(requested_fields) |
There was a problem hiding this comment.
These tests claim to validate that CSV headers “match the requested fields”, but they only assert the header count. This can pass even if the server returns the wrong fields or wrong ordering. Strengthen the assertions to compare headers to requested_fields (and, if ordering isn’t guaranteed by the API, assert set equality plus a separate ordering rule).
| text = raw.decode("utf-8-sig") | ||
| reader = csv.reader(io.StringIO(text)) | ||
| headers = next(reader) | ||
| assert len(headers) == len(requested_fields) |
There was a problem hiding this comment.
These tests claim to validate that CSV headers “match the requested fields”, but they only assert the header count. This can pass even if the server returns the wrong fields or wrong ordering. Strengthen the assertions to compare headers to requested_fields (and, if ordering isn’t guaranteed by the API, assert set equality plus a separate ordering rule).
Summary
tests/unit/manager/services/export/test_export_service.py,tests/component/scaling_group/test_scaling_group_crud.py,tests/component/export/test_export_csv.pyTest plan
pants test tests/unit/manager/services/export/test_export_service.py— 17 tests passpants test tests/unit/manager/services/image/test_image_service.py— all tests passpants test tests/unit/manager/services/container_registry/test_container_registry_service.py— all tests passpants test tests/unit/manager/services/scaling_group/test_scaling_group_service.py— all tests passpants test tests/unit/manager/services/error_log/test_error_log_service.py— all tests passpants test tests/component/scaling_group/test_scaling_group_crud.py— all tests passpants test tests/component/export/test_export_csv.py— all tests passpants fmt fix lint— all cleanResolves BA-4900