Conversation
🛡️ Bandit Security Scan Results✅ No security issues found by Bandit. |
There was a problem hiding this comment.
Pull request overview
This PR establishes a comprehensive test suite for the Codesphere Python SDK, including both unit and integration tests. The changes refactor the SDK's internal architecture to support lazy operation loading (avoiding circular imports), add extensive test coverage across all resource types, and provide integration test infrastructure with automated workspace setup/cleanup.
Changes:
- Set up complete test infrastructure with unit and integration tests covering Teams, Workspaces, Domains, Environment Variables, and Metadata resources
- Refactored SDK internals to use lazy operation loading via
_execute_operation()method, eliminating circular import issues - Added integration test workflow with session-scoped test workspace management and environment variable configuration
- Updated documentation with testing guidelines and separated command schemas to resolve import cycles
Reviewed changes
Copilot reviewed 70 out of 87 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_client.py | Migrated client tests to class-based structure with improved fixtures |
| tests/resources/workspace/test_workspace.py | Added comprehensive workspace resource unit tests |
| tests/resources/workspace/env_vars/test_env_vars.py | Added environment variables manager unit tests |
| tests/resources/team/test_team.py | Added team resource unit tests |
| tests/resources/team/domain/test_domain.py | Added domain resource unit tests |
| tests/resources/metadata/test_metadata.py | Added metadata resource unit tests |
| tests/integration/test_*.py | Added integration tests for all major resources |
| tests/integration/conftest.py | Added session-scoped workspace fixtures for integration tests |
| tests/conftest.py | Added shared test fixtures and mock factories |
| tests/resources/conftest.py | Added resource-specific test fixtures |
| src/codesphere/resources/workspace/schemas.py | Refactored to use lazy operation loading and separated command schemas |
| src/codesphere/resources/workspace/command_schemas.py | Extracted command-related schemas to avoid circular imports |
| src/codesphere/resources/workspace/envVars/schemas.py | Separated EnvVar schema to avoid circular imports |
| src/codesphere/resources/workspace/envVars/models.py | Refactored to use lazy operation loading |
| src/codesphere/resources/metadata/schemas.py | Added explicit Field validation aliases for CPU/RAM/etc fields |
| src/codesphere/core/handler.py | Enhanced null checks and removed debug logging |
| src/codesphere/core/base.py | Added serialize_by_alias configuration |
| src/codesphere/config.py | Added extra="ignore" to allow additional CS_* env vars |
| Makefile | Added test-unit and test-integration targets |
| .github/workflows/integration.yml | Added integration test GitHub Actions workflow |
| CONTRIBUTING.md | Added comprehensive testing guidelines section |
| examples/* | Removed example files (likely to be regenerated) |
| Uses plan ID 8 (Micro) which is suitable for testing. | ||
| Falls back to first non-deprecated plan if not available. | ||
| """ | ||
| plans = await session_sdk_client.metadata.list_plans() | ||
|
|
||
| # Prefer plan ID 8 (Micro) for testing |
There was a problem hiding this comment.
Using a hardcoded plan ID (8) assumes this plan exists in all environments. Consider documenting this assumption or adding a fallback if plan 8 is not found. The code does have fallbacks (lines 161-168), but the preference for ID 8 should be explained in a comment.
| Uses plan ID 8 (Micro) which is suitable for testing. | |
| Falls back to first non-deprecated plan if not available. | |
| """ | |
| plans = await session_sdk_client.metadata.list_plans() | |
| # Prefer plan ID 8 (Micro) for testing | |
| In Codesphere production/test environments, plan ID 8 is the "Micro" | |
| workspace plan, which is low-cost and suitable for automated testing. | |
| We prefer this specific plan ID when available, but do not rely on it | |
| being present: if plan 8 is missing or deprecated, we fall back to other | |
| active plans below. | |
| """ | |
| plans = await session_sdk_client.metadata.list_plans() | |
| # Prefer plan ID 8 (Micro) for testing: this ID is reserved for the | |
| # low-cost "Micro" plan in Codesphere environments, but tests still work | |
| # if it is absent thanks to the fallbacks below. |
| timestamp = int(time.time()) | ||
| return f"{TEST_DOMAIN_PREFIX}-{timestamp}.example.com" |
There was a problem hiding this comment.
The variable name 'timestamp' is misleading as this is being used as a unique identifier, not for time tracking. Consider renaming to 'unique_id' or 'suffix' to better reflect its purpose.
| timestamp = int(time.time()) | |
| return f"{TEST_DOMAIN_PREFIX}-{timestamp}.example.com" | |
| unique_suffix = int(time.time()) | |
| return f"{TEST_DOMAIN_PREFIX}-{unique_suffix}.example.com" |
| from .operations import _UPDATE_OP | ||
|
|
||
| await self._execute_operation(_UPDATE_OP, data=data) |
There was a problem hiding this comment.
The lazy import pattern is repeated across multiple methods. Consider extracting this into a helper method or documenting why operations must be imported lazily within each method rather than at module level.
| if self._http_client is None or not isinstance( | ||
| self._http_client, APIHttpClient | ||
| ): |
There was a problem hiding this comment.
This type check pattern appears in both workspace/schemas.py and team/schemas.py. Consider extracting this into a helper method in a base class to reduce duplication, such as _validate_http_client().
| cpu: float = Field(validation_alias="CPU") | ||
| gpu: int = Field(validation_alias="GPU") | ||
| ram: int = Field(validation_alias="RAM") | ||
| ssd: int = Field(validation_alias="SSD") | ||
| temp_storage: int = Field(validation_alias="TempStorage") |
There was a problem hiding this comment.
The use of uppercase validation aliases (CPU, GPU, RAM, SSD, TempStorage) deviates from the camelCase convention used elsewhere in the SDK. Consider adding a comment explaining why these specific fields require uppercase aliases, as it appears to be an API contract requirement.
| try: | ||
| workspace = await session_sdk_client.workspaces.create(payload=payload) | ||
| created_workspaces.append(workspace) | ||
| print(f"\n✓ Created test workspace: {workspace.name} (ID: {workspace.id})") |
There was a problem hiding this comment.
Using print() statements in test fixtures can clutter test output. Consider using pytest's logging mechanism or caplog fixture for better control over test output verbosity.
| self, method: str, endpoint: str, **kwargs: Any | ||
| ) -> httpx.Response: | ||
| if not self.http_client: | ||
| if self.http_client is None or not hasattr(self.http_client, "request"): |
There was a problem hiding this comment.
The check hasattr(self.http_client, 'request') is a duck typing approach. Since APIHttpClient is the expected type, consider using isinstance(self.http_client, APIHttpClient) for more explicit type checking, or document why duck typing is preferred here.
| if self.http_client is None or not hasattr(self.http_client, "request"): | |
| if not isinstance(self.http_client, APIHttpClient): |
No description provided.