Add UT for test skills for upstream-downstream tasks #867
Conversation
Summary of ChangesHello @wangxingjun778, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of unit tests for the skill DAG's upstream-downstream data passing, which is an excellent addition for ensuring the robustness of skill chaining. It also includes a necessary fix to properly sanitize special characters in skill IDs for environment variable creation. My feedback focuses on enhancing the new test file by improving test coverage for the sanitization logic and increasing type safety for better maintainability.
| import tempfile | ||
| import unittest | ||
| from pathlib import Path | ||
| from typing import Dict, List, Optional |
There was a problem hiding this comment.
To improve type safety in this file, please import Coroutine which is used as a type hint in the run_async helper function.
| from typing import Dict, List, Optional | |
| from typing import Coroutine, Dict, List, Optional |
| # Helpers | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| def run_async(coro): |
There was a problem hiding this comment.
For better type safety and readability, please add a type hint for the coro parameter.
| def run_async(coro): | |
| def run_async(coro: Coroutine): |
| executor = self.executor | ||
|
|
||
| async def mock_execute_single( | ||
| skill_id, dag, execution_input=None, query=''): |
There was a problem hiding this comment.
For better readability and maintainability, please add type hints to the parameters of this inner function.
skill_id: str, dag: Dict, execution_input: Optional[ExecutionInput] = None, query: str = ''| executor = self.executor | ||
|
|
||
| async def mock_execute_single( | ||
| skill_id, dag, execution_input=None, query=''): |
There was a problem hiding this comment.
For better readability and maintainability, please add type hints to the parameters of this inner function.
skill_id: str, dag: Dict, execution_input: Optional[ExecutionInput] = None, query: str = ''| } | ||
|
|
||
| async def mock_execute_single( | ||
| skill_id, dag, execution_input=None, query=''): |
There was a problem hiding this comment.
For better readability and maintainability, please add type hints to the parameters of this inner function.
skill_id: str, dag: Dict, execution_input: Optional[ExecutionInput] = None, query: str = ''| def test_safe_key_special_characters(self): | ||
| """Skill IDs with @, -, . are sanitized in env var names.""" | ||
| special_skill = create_mock_skill( | ||
| 'my-tool.v2@latest', 'MyTool', 'Tool with special chars', | ||
| self.skills_dir / 'my_tool') | ||
| self.skills['my-tool.v2@latest'] = special_skill | ||
|
|
||
| dep_skill = create_mock_skill( | ||
| 'consumer@latest', 'Consumer', 'Depends on special', | ||
| self.skills_dir / 'consumer') | ||
| self.skills['consumer@latest'] = dep_skill | ||
|
|
||
| self.executor._outputs['my-tool.v2@latest'] = ExecutionOutput( | ||
| stdout='special output\n', stderr='', exit_code=0, duration_ms=10) | ||
|
|
||
| dag = { | ||
| 'my-tool.v2@latest': [], | ||
| 'consumer@latest': ['my-tool.v2@latest'], | ||
| } | ||
| exec_input = self.executor._build_execution_input( | ||
| 'consumer@latest', dag) | ||
|
|
||
| # Safe key: my-tool.v2@latest -> MY_TOOL_V2_LATEST | ||
| expected_key = 'UPSTREAM_MY_TOOL_V2_LATEST_STDOUT' | ||
| self.assertIn(expected_key, exec_input.env_vars, | ||
| f'{expected_key} should be in env_vars, ' | ||
| f'got keys: {list(exec_input.env_vars.keys())}') |
There was a problem hiding this comment.
The fix in auto_skills.py adds sanitization for / characters in skill IDs, but this test only covers @, -, and .. To ensure full test coverage for the change, please update this test to include a skill ID with a / character.
def test_safe_key_special_characters(self):
"""Skill IDs with @, -, ., and / are sanitized in env var names."""
skill_id = 'my-org/my-tool.v2@latest'
special_skill = create_mock_skill(
skill_id, 'MyTool', 'Tool with special chars',
self.skills_dir / 'my_tool')
self.skills[skill_id] = special_skill
dep_skill = create_mock_skill(
'consumer@latest', 'Consumer', 'Depends on special',
self.skills_dir / 'consumer')
self.skills['consumer@latest'] = dep_skill
self.executor._outputs[skill_id] = ExecutionOutput(
stdout='special output\n', stderr='', exit_code=0, duration_ms=10)
dag = {
skill_id: [],
'consumer@latest': [skill_id],
}
exec_input = self.executor._build_execution_input(
'consumer@latest', dag)
# Safe key: my-org/my-tool.v2@latest -> MY_ORG_MY_TOOL_V2_LATEST
expected_key = 'UPSTREAM_MY_ORG_MY_TOOL_V2_LATEST_STDOUT'
self.assertIn(expected_key, exec_input.env_vars,
f'{expected_key} should be in env_vars, '
f'got keys: {list(exec_input.env_vars.keys())}')
2 participants
No description provided.