fix: Update setup_mcp.sh for v2.0.0 src/ layout + test fixes (#201)
Merges setup_mcp.sh fix for v2.0.0 src/ layout + test updates. Original fix by @501981732 in PR #197. Test updates to make CI pass. Closes #192
This commit is contained in:
yusyus
GitHub
@@ -42,7 +42,7 @@ class TestPackageSkill(unittest.TestCase):
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
skill_dir = self.create_test_skill_directory(tmpdir)
|
||||
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False)
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False, skip_quality_check=True)
|
||||
|
||||
self.assertTrue(success)
|
||||
self.assertIsNotNone(zip_path)
|
||||
@@ -55,7 +55,7 @@ class TestPackageSkill(unittest.TestCase):
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
skill_dir = self.create_test_skill_directory(tmpdir)
|
||||
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False)
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False, skip_quality_check=True)
|
||||
|
||||
self.assertTrue(success)
|
||||
|
||||
@@ -78,7 +78,7 @@ class TestPackageSkill(unittest.TestCase):
|
||||
# Add a backup file
|
||||
(skill_dir / "SKILL.md.backup").write_text("# Backup")
|
||||
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False)
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False, skip_quality_check=True)
|
||||
|
||||
self.assertTrue(success)
|
||||
|
||||
@@ -89,7 +89,7 @@ class TestPackageSkill(unittest.TestCase):
|
||||
|
||||
def test_package_nonexistent_directory(self):
|
||||
"""Test packaging a nonexistent directory"""
|
||||
success, zip_path = package_skill("/nonexistent/path", open_folder_after=False)
|
||||
success, zip_path = package_skill("/nonexistent/path", open_folder_after=False, skip_quality_check=True)
|
||||
|
||||
self.assertFalse(success)
|
||||
self.assertIsNone(zip_path)
|
||||
@@ -100,7 +100,7 @@ class TestPackageSkill(unittest.TestCase):
|
||||
skill_dir = Path(tmpdir) / "invalid-skill"
|
||||
skill_dir.mkdir()
|
||||
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False)
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False, skip_quality_check=True)
|
||||
|
||||
self.assertFalse(success)
|
||||
self.assertIsNone(zip_path)
|
||||
@@ -119,7 +119,7 @@ class TestPackageSkill(unittest.TestCase):
|
||||
(skill_dir / "scripts").mkdir()
|
||||
(skill_dir / "assets").mkdir()
|
||||
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False)
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False, skip_quality_check=True)
|
||||
|
||||
self.assertTrue(success)
|
||||
# Zip should be in output directory, not inside skill directory
|
||||
@@ -136,7 +136,7 @@ class TestPackageSkill(unittest.TestCase):
|
||||
(skill_dir / "scripts").mkdir()
|
||||
(skill_dir / "assets").mkdir()
|
||||
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False)
|
||||
success, zip_path = package_skill(skill_dir, open_folder_after=False, skip_quality_check=True)
|
||||
|
||||
self.assertTrue(success)
|
||||
self.assertEqual(zip_path.name, "my-awesome-skill.zip")
|
||||
|
||||
297
tests/test_quality_checker.py
Normal file
297
tests/test_quality_checker.py
Normal file
@@ -0,0 +1,297 @@
|
||||
#!/usr/bin/env python3
|
||||
"""
|
||||
Tests for cli/quality_checker.py functionality
|
||||
"""
|
||||
|
||||
import unittest
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
import os
|
||||
|
||||
from skill_seekers.cli.quality_checker import SkillQualityChecker, QualityReport
|
||||
|
||||
|
||||
class TestQualityChecker(unittest.TestCase):
|
||||
"""Test quality checker functionality"""
|
||||
|
||||
def create_test_skill(self, tmpdir, skill_md_content, create_references=True):
|
||||
"""Helper to create a test skill directory"""
|
||||
skill_dir = Path(tmpdir) / "test-skill"
|
||||
skill_dir.mkdir()
|
||||
|
||||
# Create SKILL.md
|
||||
skill_md = skill_dir / "SKILL.md"
|
||||
skill_md.write_text(skill_md_content, encoding='utf-8')
|
||||
|
||||
# Create references directory
|
||||
if create_references:
|
||||
refs_dir = skill_dir / "references"
|
||||
refs_dir.mkdir()
|
||||
(refs_dir / "index.md").write_text("# Index\n\nTest reference.", encoding='utf-8')
|
||||
(refs_dir / "getting_started.md").write_text("# Getting Started\n\nHow to start.", encoding='utf-8')
|
||||
|
||||
return skill_dir
|
||||
|
||||
def test_checker_detects_missing_skill_md(self):
|
||||
"""Test that checker detects missing SKILL.md"""
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
skill_dir = Path(tmpdir) / "test-skill"
|
||||
skill_dir.mkdir()
|
||||
|
||||
checker = SkillQualityChecker(skill_dir)
|
||||
report = checker.check_all()
|
||||
|
||||
# Should have error about missing SKILL.md
|
||||
self.assertTrue(report.has_errors)
|
||||
self.assertTrue(any('SKILL.md' in issue.message for issue in report.errors))
|
||||
|
||||
def test_checker_detects_missing_references(self):
|
||||
"""Test that checker warns about missing references"""
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
skill_md = """---
|
||||
name: test
|
||||
---
|
||||
|
||||
# Test Skill
|
||||
|
||||
This is a test.
|
||||
"""
|
||||
skill_dir = self.create_test_skill(tmpdir, skill_md, create_references=False)
|
||||
|
||||
checker = SkillQualityChecker(skill_dir)
|
||||
report = checker.check_all()
|
||||
|
||||
# Should have warning about missing references
|
||||
self.assertTrue(report.has_warnings)
|
||||
self.assertTrue(any('references' in issue.message.lower() for issue in report.warnings))
|
||||
|
||||
def test_checker_detects_invalid_frontmatter(self):
|
||||
"""Test that checker detects invalid YAML frontmatter"""
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
skill_md = """# Test Skill
|
||||
|
||||
No frontmatter here!
|
||||
"""
|
||||
skill_dir = self.create_test_skill(tmpdir, skill_md)
|
||||
|
||||
checker = SkillQualityChecker(skill_dir)
|
||||
report = checker.check_all()
|
||||
|
||||
# Should have error about missing frontmatter
|
||||
self.assertTrue(report.has_errors)
|
||||
self.assertTrue(any('frontmatter' in issue.message.lower() for issue in report.errors))
|
||||
|
||||
def test_checker_detects_missing_name_field(self):
|
||||
"""Test that checker detects missing name field in frontmatter"""
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
skill_md = """---
|
||||
description: test
|
||||
---
|
||||
|
||||
# Test Skill
|
||||
"""
|
||||
skill_dir = self.create_test_skill(tmpdir, skill_md)
|
||||
|
||||
checker = SkillQualityChecker(skill_dir)
|
||||
report = checker.check_all()
|
||||
|
||||
# Should have error about missing name field
|
||||
self.assertTrue(report.has_errors)
|
||||
self.assertTrue(any('name' in issue.message.lower() for issue in report.errors))
|
||||
|
||||
def test_checker_detects_code_without_language(self):
|
||||
"""Test that checker warns about code blocks without language tags"""
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
skill_md = """---
|
||||
name: test
|
||||
---
|
||||
|
||||
# Test Skill
|
||||
|
||||
Here's some code:
|
||||
|
||||
```
|
||||
print("hello")
|
||||
```
|
||||
"""
|
||||
skill_dir = self.create_test_skill(tmpdir, skill_md)
|
||||
|
||||
checker = SkillQualityChecker(skill_dir)
|
||||
report = checker.check_all()
|
||||
|
||||
# Should have warning about code without language
|
||||
self.assertTrue(report.has_warnings)
|
||||
self.assertTrue(any('language' in issue.message.lower() for issue in report.warnings))
|
||||
|
||||
def test_checker_approves_good_skill(self):
|
||||
"""Test that checker gives high score to well-formed skill"""
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
skill_md = """---
|
||||
name: test
|
||||
description: A test skill
|
||||
---
|
||||
|
||||
# Test Skill
|
||||
|
||||
## When to Use This Skill
|
||||
|
||||
Use this when you need to test.
|
||||
|
||||
## Quick Reference
|
||||
|
||||
Here are some examples:
|
||||
|
||||
```python
|
||||
def hello():
|
||||
print("hello")
|
||||
```
|
||||
|
||||
```javascript
|
||||
console.log("hello");
|
||||
```
|
||||
|
||||
## Example: Basic Usage
|
||||
|
||||
This shows how to use it.
|
||||
|
||||
## Reference Files
|
||||
|
||||
See the references directory for more:
|
||||
- [Getting Started](references/getting_started.md)
|
||||
- [Index](references/index.md)
|
||||
"""
|
||||
skill_dir = self.create_test_skill(tmpdir, skill_md)
|
||||
|
||||
checker = SkillQualityChecker(skill_dir)
|
||||
report = checker.check_all()
|
||||
|
||||
# Should have no errors
|
||||
self.assertFalse(report.has_errors)
|
||||
|
||||
# Quality score should be high
|
||||
self.assertGreaterEqual(report.quality_score, 80.0)
|
||||
|
||||
def test_checker_detects_broken_links(self):
|
||||
"""Test that checker detects broken internal links"""
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
skill_md = """---
|
||||
name: test
|
||||
---
|
||||
|
||||
# Test Skill
|
||||
|
||||
See [this file](nonexistent.md) for more info.
|
||||
"""
|
||||
skill_dir = self.create_test_skill(tmpdir, skill_md)
|
||||
|
||||
checker = SkillQualityChecker(skill_dir)
|
||||
report = checker.check_all()
|
||||
|
||||
# Should have warning about broken link
|
||||
self.assertTrue(report.has_warnings)
|
||||
self.assertTrue(any('broken link' in issue.message.lower() for issue in report.warnings))
|
||||
|
||||
def test_quality_score_calculation(self):
|
||||
"""Test that quality score is calculated correctly"""
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
report = QualityReport("test", Path(tmpdir))
|
||||
|
||||
# Perfect score to start
|
||||
self.assertEqual(report.quality_score, 100.0)
|
||||
|
||||
# Add an error (should deduct 15 points)
|
||||
report.add_error('test', 'Test error')
|
||||
self.assertEqual(report.quality_score, 85.0)
|
||||
|
||||
# Add a warning (should deduct 5 points)
|
||||
report.add_warning('test', 'Test warning')
|
||||
self.assertEqual(report.quality_score, 80.0)
|
||||
|
||||
# Add more errors
|
||||
report.add_error('test', 'Another error')
|
||||
report.add_error('test', 'Yet another error')
|
||||
self.assertEqual(report.quality_score, 50.0)
|
||||
|
||||
def test_quality_grade_calculation(self):
|
||||
"""Test that quality grades are assigned correctly"""
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
report = QualityReport("test", Path(tmpdir))
|
||||
|
||||
# Grade A (90-100)
|
||||
self.assertEqual(report.quality_grade, 'A')
|
||||
|
||||
# Grade B (80-89)
|
||||
report.add_error('test', 'Error 1')
|
||||
self.assertEqual(report.quality_grade, 'B')
|
||||
|
||||
# Grade C (70-79)
|
||||
report.add_warning('test', 'Warning 1')
|
||||
report.add_warning('test', 'Warning 2')
|
||||
self.assertEqual(report.quality_grade, 'C')
|
||||
|
||||
# Grade D (60-69)
|
||||
report.add_warning('test', 'Warning 3')
|
||||
report.add_warning('test', 'Warning 4')
|
||||
self.assertEqual(report.quality_grade, 'D')
|
||||
|
||||
# Grade F (below 60)
|
||||
report.add_error('test', 'Error 2')
|
||||
report.add_error('test', 'Error 3')
|
||||
self.assertEqual(report.quality_grade, 'F')
|
||||
|
||||
def test_is_excellent_property(self):
|
||||
"""Test is_excellent property"""
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
report = QualityReport("test", Path(tmpdir))
|
||||
|
||||
# Should be excellent with no issues
|
||||
self.assertTrue(report.is_excellent)
|
||||
|
||||
# Adding an error should make it not excellent
|
||||
report.add_error('test', 'Test error')
|
||||
self.assertFalse(report.is_excellent)
|
||||
|
||||
# Clean report
|
||||
report2 = QualityReport("test", Path(tmpdir))
|
||||
# Adding a warning should also make it not excellent
|
||||
report2.add_warning('test', 'Test warning')
|
||||
self.assertFalse(report2.is_excellent)
|
||||
|
||||
|
||||
class TestQualityCheckerCLI(unittest.TestCase):
|
||||
"""Test quality checker CLI"""
|
||||
|
||||
def test_cli_help_output(self):
|
||||
"""Test that CLI help works"""
|
||||
import subprocess
|
||||
|
||||
try:
|
||||
result = subprocess.run(
|
||||
['python3', '-m', 'skill_seekers.cli.quality_checker', '--help'],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=5
|
||||
)
|
||||
|
||||
# Should include usage info
|
||||
output = result.stdout + result.stderr
|
||||
self.assertTrue('usage:' in output.lower() or 'quality' in output.lower())
|
||||
except FileNotFoundError:
|
||||
self.skipTest("Module not installed")
|
||||
|
||||
def test_cli_with_nonexistent_directory(self):
|
||||
"""Test CLI behavior with nonexistent directory"""
|
||||
import subprocess
|
||||
|
||||
result = subprocess.run(
|
||||
['python3', '-m', 'skill_seekers.cli.quality_checker', '/nonexistent/path'],
|
||||
capture_output=True,
|
||||
text=True
|
||||
)
|
||||
|
||||
# Should fail
|
||||
self.assertNotEqual(result.returncode, 0)
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
unittest.main()
|
||||
@@ -40,34 +40,50 @@ class TestSetupMCPScript:
|
||||
assert result.returncode == 0, f"Bash syntax error: {result.stderr}"
|
||||
|
||||
def test_references_correct_mcp_directory(self, script_content):
|
||||
"""Test that script references skill_seeker_mcp/ not old mcp/ directory"""
|
||||
# Should NOT reference old mcp/ directory
|
||||
old_refs = re.findall(r'(?:^|[^a-z_])mcp/(?!\.json)', script_content, re.MULTILINE)
|
||||
assert len(old_refs) == 0, f"Found {len(old_refs)} references to old 'mcp/' directory: {old_refs}"
|
||||
"""Test that script references src/skill_seekers/mcp/ (v2.0.0 layout)"""
|
||||
# Should NOT reference old mcp/ or skill_seeker_mcp/ directories
|
||||
old_mcp_refs = re.findall(r'(?:^|[^a-z_])(?<!/)mcp/(?!\.json)', script_content, re.MULTILINE)
|
||||
old_skill_seeker_refs = re.findall(r'skill_seeker_mcp/', script_content)
|
||||
|
||||
# SHOULD reference skill_seeker_mcp/
|
||||
new_refs = re.findall(r'skill_seeker_mcp/', script_content)
|
||||
assert len(new_refs) >= 6, f"Expected at least 6 references to 'skill_seeker_mcp/', found {len(new_refs)}"
|
||||
# Allow /mcp/ (as in src/skill_seekers/mcp/) but not standalone mcp/
|
||||
assert len(old_mcp_refs) == 0, f"Found {len(old_mcp_refs)} references to old 'mcp/' directory: {old_mcp_refs}"
|
||||
assert len(old_skill_seeker_refs) == 0, f"Found {len(old_skill_seeker_refs)} references to old 'skill_seeker_mcp/': {old_skill_seeker_refs}"
|
||||
|
||||
# SHOULD reference src/skill_seekers/mcp/
|
||||
new_refs = re.findall(r'src/skill_seekers/mcp/', script_content)
|
||||
assert len(new_refs) >= 6, f"Expected at least 6 references to 'src/skill_seekers/mcp/', found {len(new_refs)}"
|
||||
|
||||
def test_requirements_txt_path(self, script_content):
|
||||
"""Test that requirements.txt path is correct"""
|
||||
assert "skill_seeker_mcp/requirements.txt" in script_content, \
|
||||
"Should reference skill_seeker_mcp/requirements.txt"
|
||||
# Check for old mcp/ directory (but not skill_seeker_mcp/)
|
||||
"""Test that script uses pip install -e . (v2.0.0 modern packaging)"""
|
||||
# v2.0.0 uses '-e .' (editable install) instead of requirements files
|
||||
# The actual command is "$PIP_INSTALL_CMD -e ."
|
||||
assert " -e ." in script_content or " -e." in script_content, \
|
||||
"Should use '-e .' for editable install (modern packaging)"
|
||||
|
||||
# Should NOT reference old requirements.txt paths
|
||||
import re
|
||||
old_refs = re.findall(r'(?<!skill_seeker_)mcp/requirements\.txt', script_content)
|
||||
assert len(old_refs) == 0, \
|
||||
f"Should NOT reference old 'mcp/requirements.txt' (found {len(old_refs)}): {old_refs}"
|
||||
old_skill_seeker_refs = re.findall(r'skill_seeker_mcp/requirements\.txt', script_content)
|
||||
old_mcp_refs = re.findall(r'(?<!skill_seeker_)mcp/requirements\.txt', script_content)
|
||||
|
||||
assert len(old_skill_seeker_refs) == 0, \
|
||||
f"Should NOT reference 'skill_seeker_mcp/requirements.txt' (found {len(old_skill_seeker_refs)})"
|
||||
assert len(old_mcp_refs) == 0, \
|
||||
f"Should NOT reference old 'mcp/requirements.txt' (found {len(old_mcp_refs)})"
|
||||
|
||||
def test_server_py_path(self, script_content):
|
||||
"""Test that server.py path is correct"""
|
||||
"""Test that server.py path is correct (v2.0.0 layout)"""
|
||||
import re
|
||||
assert "skill_seeker_mcp/server.py" in script_content, \
|
||||
"Should reference skill_seeker_mcp/server.py"
|
||||
# Check for old mcp/ directory (but not skill_seeker_mcp/)
|
||||
old_refs = re.findall(r'(?<!skill_seeker_)mcp/server\.py', script_content)
|
||||
assert len(old_refs) == 0, \
|
||||
f"Should NOT reference old 'mcp/server.py' (found {len(old_refs)}): {old_refs}"
|
||||
assert "src/skill_seekers/mcp/server.py" in script_content, \
|
||||
"Should reference src/skill_seekers/mcp/server.py"
|
||||
|
||||
# Should NOT reference old paths
|
||||
old_skill_seeker_refs = re.findall(r'skill_seeker_mcp/server\.py', script_content)
|
||||
old_mcp_refs = re.findall(r'(?<!/)(?<!skill_seekers/)mcp/server\.py', script_content)
|
||||
|
||||
assert len(old_skill_seeker_refs) == 0, \
|
||||
f"Should NOT reference old 'skill_seeker_mcp/server.py' (found {len(old_skill_seeker_refs)})"
|
||||
assert len(old_mcp_refs) == 0, \
|
||||
f"Should NOT reference old 'mcp/server.py' (found {len(old_mcp_refs)})"
|
||||
|
||||
def test_referenced_files_exist(self):
|
||||
"""Test that all files referenced in setup_mcp.sh actually exist"""
|
||||
@@ -88,10 +104,10 @@ class TestSetupMCPScript:
|
||||
assert os.access(script_path, os.X_OK), "setup_mcp.sh should be executable"
|
||||
|
||||
def test_json_config_path_format(self, script_content):
|
||||
"""Test that JSON config examples use correct format"""
|
||||
"""Test that JSON config examples use correct format (v2.0.0 layout)"""
|
||||
# Check for the config path format in the script
|
||||
assert '"$REPO_PATH/skill_seeker_mcp/server.py"' in script_content, \
|
||||
"Config should show correct server.py path with $REPO_PATH variable"
|
||||
assert '"$REPO_PATH/src/skill_seekers/mcp/server.py"' in script_content, \
|
||||
"Config should show correct server.py path with $REPO_PATH variable (v2.0.0 layout)"
|
||||
|
||||
def test_no_hardcoded_paths(self, script_content):
|
||||
"""Test that script doesn't contain hardcoded absolute paths"""
|
||||
|
||||
@@ -164,9 +164,9 @@ class TestDetectTerminalApp(unittest.TestCase):
|
||||
# Mock Popen to prevent actual terminal launch
|
||||
mock_popen.return_value = MagicMock()
|
||||
|
||||
# Run enhancer
|
||||
# Run enhancer in interactive mode (not headless)
|
||||
enhancer = LocalSkillEnhancer(skill_dir)
|
||||
result = enhancer.run()
|
||||
result = enhancer.run(headless=False)
|
||||
|
||||
# Verify Popen was called
|
||||
self.assertTrue(mock_popen.called)
|
||||
@@ -239,7 +239,8 @@ class TestDetectTerminalApp(unittest.TestCase):
|
||||
old_stdout = sys.stdout
|
||||
sys.stdout = captured_output
|
||||
|
||||
result = enhancer.run()
|
||||
# Run in interactive mode (not headless) to test terminal launch
|
||||
result = enhancer.run(headless=False)
|
||||
|
||||
# Restore stdout
|
||||
sys.stdout = old_stdout
|
||||
@@ -279,7 +280,8 @@ class TestDetectTerminalApp(unittest.TestCase):
|
||||
# Mock Popen to prevent actual launch
|
||||
with patch('subprocess.Popen') as mock_popen:
|
||||
mock_popen.return_value = MagicMock()
|
||||
enhancer.run()
|
||||
# Run in interactive mode (not headless) to test terminal detection
|
||||
enhancer.run(headless=False)
|
||||
|
||||
# Restore stdout
|
||||
sys.stdout = old_stdout
|
||||
|
||||
Reference in New Issue
Block a user