[feat] Enable lean testset revisions #3219
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR enables lean testset revisions by adding an optional include_testcases parameter to all testset revision endpoints. This allows clients to request either full testcase objects (default, backward-compatible) or just testcase IDs for improved performance in list views and metadata operations.
- Adds
include_testcasesparameter to all revision endpoints (retrieve, create, fetch, edit, query, commit, log) - Refactors testcase population logic into a reusable
_populate_testcaseshelper method - Implements automatic variant resolution when only
artifact_idis provided in log operations - Renames
TestsetLogtoTestsetRevisionsLogfor better consistency with the domain model
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/oss/tests/manual/testsets/testcase-inclusion.http | Manual test scenarios demonstrating the new include_testcases parameter functionality across all affected endpoints |
| api/oss/src/dbs/postgres/git/dao.py | Adds logic to fetch default variant when only artifact_id is provided in log_revisions |
| api/oss/src/core/testsets/service.py | Introduces _populate_testcases helper method and updates all revision methods to support the include_testcases parameter |
| api/oss/src/core/testsets/dtos.py | Renames TestsetLog to TestsetRevisionsLog and adds testset_id field with proper alias synchronization |
| api/oss/src/apis/fastapi/testsets/utils.py | Updates request parsing functions to accept and pass through the include_testcases parameter |
| api/oss/src/apis/fastapi/testsets/router.py | Adds include_testcases parameter to all revision endpoints and configures response model excludes for alias fields |
| api/oss/src/apis/fastapi/testsets/models.py | Adds include_testcases field to all revision request models and renames TestsetLogRequest to TestsetRevisionsLogRequest |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| testset_revision: The testset revision to populate | ||
| project_id: Project ID for fetching testcases |
There was a problem hiding this comment.
The docstring has inconsistent argument ordering. The docstring lists testset_revision first, then project_id, then include_testcases, but the actual function signature has project_id first, then testset_revision, then include_testcases. The docstring should match the actual parameter order.
| testset_revision: The testset revision to populate | |
| project_id: Project ID for fetching testcases | |
| project_id: Project ID for fetching testcases | |
| testset_revision: The testset revision to populate |
| async def _populate_testcases( | ||
| self, | ||
| project_id: UUID, | ||
| # | ||
| testset_revision: TestsetRevision, | ||
| include_testcases: Optional[bool], | ||
| ) -> None: | ||
| """Conditionally populate testcases in revision data. | ||
| Args: | ||
| testset_revision: The testset revision to populate | ||
| project_id: Project ID for fetching testcases | ||
| include_testcases: If None or True, fetch and include testcases. | ||
| If False, leave testcases as None (only testcase_ids). | ||
| """ | ||
| if not testset_revision.data: | ||
| return | ||
|
|
||
| # Default to True if None (backward compatible) | ||
| if include_testcases is None or include_testcases: | ||
| # Include full testcases, exclude testcase_ids | ||
| if testset_revision.data.testcase_ids: | ||
| testset_revision.data.testcases = ( | ||
| await self.testcases_service.fetch_testcases( | ||
| project_id=project_id, | ||
| testcase_ids=testset_revision.data.testcase_ids, | ||
| ) | ||
| ) | ||
| # Clear testcase_ids when including full testcases | ||
| testset_revision.data.testcase_ids = None | ||
|
|
||
| # Clear alias fields from testcases for clean API responses | ||
| if testset_revision.data.testcases: | ||
| for testcase in testset_revision.data.testcases: | ||
| testcase.set_id = None | ||
| else: | ||
| # Include only testcase_ids, exclude full testcases | ||
| testset_revision.data.testcases = None | ||
|
|
There was a problem hiding this comment.
The new include_testcases parameter functionality for testset revision endpoints lacks automated test coverage. Consider adding pytest tests to verify that: 1) when include_testcases is None or True, full testcase objects are returned; 2) when include_testcases is False, only testcase_ids are returned and testcases is None; and 3) the response sizes differ as expected. This is important since other endpoints in this directory have comprehensive pytest coverage.
No description provided.