-
Notifications
You must be signed in to change notification settings - Fork 5.7k
fix(tests): resolve test failures across framework and tools #7059
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,6 @@ | |
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
| import importlib | ||
| import json | ||
| import os | ||
| import shutil | ||
|
|
@@ -53,69 +52,26 @@ def _has_any_llm_key() -> bool: | |
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Fixture: isolated ~/.hive in a temp dir | ||
| # Fixture: copy real LLM config into the conftest-provided isolated ~/.hive | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| # Modules that import HIVE_HOME / QUEENS_DIR / COLONIES_DIR / MEMORIES_DIR / | ||
| # HIVE_CONFIG_FILE at import time and need their bindings rewritten when we | ||
| # redirect ~/.hive to a temp directory. | ||
| _HIVE_PATH_CONSUMERS = ( | ||
| "framework.config", | ||
| "framework.server.session_manager", | ||
| "framework.server.queen_orchestrator", | ||
| "framework.server.routes_queens", | ||
| "framework.server.app", | ||
| "framework.agents.discovery", | ||
| "framework.agents.queen.queen_profiles", | ||
| "framework.tools.queen_lifecycle_tools", | ||
| "framework.storage.migrate_v2", | ||
| "framework.loader.cli", | ||
| ) | ||
|
|
||
| _HIVE_PATH_NAMES = ( | ||
| ("HIVE_HOME", lambda h: h), | ||
| ("QUEENS_DIR", lambda h: h / "agents" / "queens"), | ||
| ("COLONIES_DIR", lambda h: h / "colonies"), | ||
| ("MEMORIES_DIR", lambda h: h / "memories"), | ||
| ("HIVE_CONFIG_FILE", lambda h: h / "configuration.json"), | ||
| ) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def isolated_hive_home(tmp_path, monkeypatch): | ||
| """Redirect ~/.hive to a temp directory. | ||
|
|
||
| Patches Path.home() AND every module-level binding of HIVE_HOME and | ||
| its derivatives, since those constants were captured at import time | ||
| and won't follow Path.home() changes alone. | ||
| def isolated_hive_home(_isolate_hive_home_autouse): | ||
| """Extend the conftest autouse fixture with the user's LLM configuration. | ||
|
|
||
| Copies the user's real ~/.hive/configuration.json into the temp home | ||
| so the LLM provider config (model, api_base) is preserved. | ||
| The conftest ``_isolate_hive_home_autouse`` fixture already redirects | ||
| ``~/.hive`` to a temp directory and patches all module-level path | ||
| constants. This fixture just copies the real ``configuration.json`` | ||
| so the live integration test can pick up API keys and model config. | ||
| """ | ||
| fake_home_root = tmp_path | ||
| fake_hive = fake_home_root / ".hive" | ||
| fake_hive.mkdir() | ||
| fake_hive = _isolate_hive_home_autouse | ||
|
|
||
| # Copy LLM configuration so the framework picks up the user's model. | ||
| # Done BEFORE we monkey-patch Path.home so the source resolves correctly. | ||
| real_config = Path.home() / ".hive" / "configuration.json" | ||
| # Use os.path.expanduser since Path.home() is already patched by conftest. | ||
| real_config = Path(os.path.expanduser("~/.hive/configuration.json")) | ||
| if real_config.exists(): | ||
| shutil.copy(real_config, fake_hive / "configuration.json") | ||
|
|
||
| # Patch Path.home -> tmp_path so any call-site computation goes there. | ||
| monkeypatch.setattr(Path, "home", classmethod(lambda cls: fake_home_root)) | ||
|
|
||
| # Patch every module-level binding that captured a path constant. | ||
| for mod_name in _HIVE_PATH_CONSUMERS: | ||
| try: | ||
| mod = importlib.import_module(mod_name) | ||
| except ImportError: | ||
| continue | ||
| for attr_name, builder in _HIVE_PATH_NAMES: | ||
| if hasattr(mod, attr_name): | ||
| monkeypatch.setattr(mod, attr_name, builder(fake_hive)) | ||
|
|
||
| yield fake_hive | ||
|
Comment on lines
+60
to
75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
python - <<'PY'
import ast
from pathlib import Path
path = Path("core/tests/test_colony_fork_live.py")
tree = ast.parse(path.read_text(encoding="utf-8"))
for node in tree.body:
if isinstance(node, ast.FunctionDef) and node.name == "isolated_hive_home":
missing_args = [a.arg for a in node.args.args if a.annotation is None]
print("function:", node.name)
print("missing_arg_annotations:", missing_args)
print("missing_return_annotation:", node.returns is None)
PYRepository: aden-hive/hive Length of output: 174 🏁 Script executed: # Find and examine the _isolate_hive_home_autouse fixture definition
fd -t f 'conftest.py' -x grep -l '_isolate_hive_home_autouse' {}Repository: aden-hive/hive Length of output: 82 🏁 Script executed: # Examine the conftest.py file to find the fixture definition
cat -n ./core/tests/conftest.py | grep -A 20 '_isolate_hive_home_autouse'Repository: aden-hive/hive Length of output: 1177 🏁 Script executed: # Get more context to see what the fixture returns/yields
cat -n ./core/tests/conftest.py | sed -n '63,95p'Repository: aden-hive/hive Length of output: 1204 🏁 Script executed: # Check the actual function and its current imports at the top of the file
cat -n ./core/tests/test_colony_fork_live.py | head -80Repository: aden-hive/hive Length of output: 3128 Add type hints to the fixture signature. Line 60 defines ♻️ Proposed fix import asyncio
+from collections.abc import Iterator
import json
import os
import shutil
import time
from pathlib import Path
@@
`@pytest.fixture`
-def isolated_hive_home(_isolate_hive_home_autouse):
+def isolated_hive_home(_isolate_hive_home_autouse: Path) -> Iterator[Path]:🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -17,10 +17,10 @@ | |||||
|
|
||||||
|
|
||||||
| class TestDefaultSkillFiles: | ||||||
| """Verify all 6 built-in SKILL.md files parse correctly.""" | ||||||
| """Verify all 7 built-in SKILL.md files parse correctly.""" | ||||||
|
|
||||||
| def test_all_six_skills_exist(self): | ||||||
| assert len(SKILL_REGISTRY) == 6 | ||||||
| def test_all_seven_skills_exist(self): | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add return type hint to the renamed test method. Line 22 introduces a changed function signature without an explicit type hint. Suggested fix- def test_all_seven_skills_exist(self):
+ def test_all_seven_skills_exist(self) -> None:As per coding guidelines, "Use type hints on all function signatures". 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| assert len(SKILL_REGISTRY) == 7 | ||||||
|
|
||||||
| @pytest.mark.parametrize("skill_name,dir_name", list(SKILL_REGISTRY.items())) | ||||||
| def test_skill_parses(self, skill_name, dir_name): | ||||||
|
|
@@ -35,7 +35,7 @@ def test_skill_parses(self, skill_name, dir_name): | |||||
| assert parsed.source_scope == "framework" | ||||||
|
|
||||||
| def test_combined_token_budget(self): | ||||||
| """All default skill bodies combined should be under 2000 tokens (~8000 chars).""" | ||||||
| """All default skill bodies combined should be under 3000 tokens (~12000 chars).""" | ||||||
| total_chars = 0 | ||||||
| for dir_name in SKILL_REGISTRY.values(): | ||||||
| path = _DEFAULT_SKILLS_DIR / dir_name / "SKILL.md" | ||||||
|
|
@@ -44,9 +44,9 @@ def test_combined_token_budget(self): | |||||
| total_chars += len(parsed.body) | ||||||
|
|
||||||
| approx_tokens = total_chars // 4 | ||||||
| assert approx_tokens < 2000, ( | ||||||
| assert approx_tokens < 3000, ( | ||||||
| f"Combined default skill bodies are ~{approx_tokens} tokens " | ||||||
| f"({total_chars} chars), exceeding the 2000 token budget" | ||||||
| f"({total_chars} chars), exceeding the 3000 token budget" | ||||||
| ) | ||||||
|
|
||||||
| def test_data_buffer_keys_all_prefixed(self): | ||||||
|
|
@@ -60,7 +60,7 @@ def test_load_all_defaults(self): | |||||
| manager = DefaultSkillManager() | ||||||
| manager.load() | ||||||
|
|
||||||
| assert len(manager.active_skill_names) == 6 | ||||||
| assert len(manager.active_skill_names) == 7 | ||||||
| for name in SKILL_REGISTRY: | ||||||
| assert name in manager.active_skill_names | ||||||
|
|
||||||
|
|
@@ -97,7 +97,7 @@ def test_disable_single_skill(self): | |||||
| manager.load() | ||||||
|
|
||||||
| assert "hive.quality-monitor" not in manager.active_skill_names | ||||||
| assert len(manager.active_skill_names) == 5 | ||||||
| assert len(manager.active_skill_names) == 6 | ||||||
|
|
||||||
| def test_disable_all_via_convention(self): | ||||||
| config = SkillsConfig.from_agent_vars(default_skills={"_all": {"enabled": False}}) | ||||||
|
|
@@ -231,11 +231,17 @@ def test_context_preservation_override_threshold(self): | |||||
| assert "45%" not in prompt | ||||||
|
|
||||||
| def test_no_unreplaced_placeholders_with_defaults(self): | ||||||
| """All {{...}} placeholders should be replaced when using defaults.""" | ||||||
| """All {{...}} placeholders should be replaced when using defaults. | ||||||
|
|
||||||
| The writing-hive-skills skill contains literal ``{{placeholder}}`` | ||||||
| as documentation text, so we strip that known occurrence before checking. | ||||||
| """ | ||||||
| manager = DefaultSkillManager() | ||||||
| manager.load() | ||||||
| prompt = manager.build_protocols_prompt() | ||||||
| assert "{{" not in prompt | ||||||
| # Remove the known literal {{placeholder}} documentation example | ||||||
| cleaned = prompt.replace("{{placeholder}}", "") | ||||||
| assert "{{" not in cleaned | ||||||
|
Comment on lines
+242
to
+244
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Current placeholder cleanup can mask real regressions.
Suggested fix- # Remove the known literal {{placeholder}} documentation example
- cleaned = prompt.replace("{{placeholder}}", "")
+ # Remove exactly one known literal {{placeholder}} documentation example
+ literal_placeholder = "{{placeholder}}"
+ assert prompt.count(literal_placeholder) == 1
+ cleaned = prompt.replace(literal_placeholder, "", 1)
assert "{{" not in cleaned🤖 Prompt for AI Agents |
||||||
|
|
||||||
|
|
||||||
| class TestBatchAutoDetection: | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle bracketless
"Old tool result"placeholders too.On Line 83,
microcompact()skips"[Old tool result"but not"Old tool result"(the format this file currently writes). That allows already-cleared spillover placeholders to be compacted again.Suggested fix
🤖 Prompt for AI Agents