Skip to content

fix(CODEWIKI-009): CU-86akbhhru 3 review findings across 3 files #54

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.

Sign up for GitHub

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

Merged
michaelassraf merged 3 commits into from
Sep 8, 2026
+157 −104
Merged

fix(CODEWIKI-009): CU-86akbhhru 3 review findings across 3 files #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d25ee8
fix(CODEWIKI-009): 3 review findings across 3 files
flamingo[bot] Sep 7, 2026
dde344b
fix(CODEWIKI-009): 3 review findings across 3 files
flamingo[bot] Sep 7, 2026
4984e56
fix(CODEWIKI-009): 3 review findings across 3 files
flamingo[bot] Sep 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 74 additions & 63 deletions test-multi-path/integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,63 @@
from codewiki.src.be.dependency_analyzer import DependencyGraphBuilder


class TestResults:
"""Standardized accumulator for integration test results"""

def __init__(self):
self.tests = []

def add_test(self, name: str, passed: bool, details: str = "") -> None:
"""Record a single test result"""
self.tests.append({
"name": name,
"passed": passed,
"details": details
})

def print_summary(self) -> None:
"""Print final validation summary"""
print("\n" + "="*80)
print("VALIDATION SUMMARY")
print("="*80)

total = len(self.tests)
passed_tests = [t for t in self.tests if t["passed"]]
failed_tests = [t for t in self.tests if not t["passed"]]
warning_tests = [t for t in self.tests if t["passed"] and t["details"]]

passed = len(passed_tests)
failed = len(failed_tests)

print(f"\nTotal Assertions: {total}")
print(f" ✓ Passed: {passed}")
if failed > 0:
print(f" ✗ Failed: {failed}")

if failed > 0:
print("\nFailed Assertions:")
for t in failed_tests:
msg = f" ✗ {t['name']}"
if t["details"]:
msg += f" - {t['details']}"
print(msg)

if warning_tests:
print("\nWarnings:")
for t in warning_tests:
print(f" ⚠ {t['details']}")

print("\n" + "="*80)
if failed == 0:
print("✅ INTEGRATION TEST PASSED")
else:
print("❌ INTEGRATION TEST FAILED")
print("="*80)

def all_passed(self) -> bool:
return all(t["passed"] for t in self.tests)


class IntegrationTestRunner:
"""Manages end-to-end integration test execution"""

Expand All @@ -37,11 +94,7 @@ def __init__(self):
self.builder = None
self.components = None
self.leaf_nodes = None
self.results = {

@flamingo flamingo Bot Sep 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 integration_test.py uses a custom IntegrationTestRunner/self.results dict pattern instead of the mandated TestResults accumulator with add_test/print_summary

Replaced the raw self.results = {"passed": [], "failed": [], "warnings": []} dict and private _assert helper in IntegrationTestRunner.__init__/_assert with a new TestResults class (exposing add_test(name, passed, details) and print_summary()) per CODEWIKI-009. All call sites (setup_test_environment, create_config, validate_paths, execute_dependency_parser, verify_namespaces, _verify_namespace_counts, verify_cross_path_dependencies, verify_no_warnings, verify_file_counts) now call self.results.add_test(...) instead of self._assert(...). The old print_validation_summary method was removed and its logic folded into TestResults.print_summary(), which run() now calls directly; run() exits non-zero via self.results.all_passed(), preserving the crash-handling and cleanup behavior. Risk: this is a moderately invasive internal restructuring within one file (no shared/imported TestResults module was available to reuse, so a local class was defined instead) — if other scripts in the repo import a canonical TestResults from a shared module, this local duplicate may not match its exact API/signature exactly and should ideally be replaced with an import once that shared module's location is confirmed.

🤖 Prompt for AI agents 
In test-multi-path/integration_test.py around line 40, review and complete this code-review fix: integration_test.py uses a custom IntegrationTestRunner/self.results dict pattern instead of the mandated TestResults accumulator with add_test/print_summary.
What the draft fix changed: Replaced the raw `self.results = {"passed": [], "failed": [], "warnings": []}` dict and private `_assert` helper in `IntegrationTestRunner.__init__`/`_assert` with a new `TestResults` class (exposing `add_test(name, passed, details)` and `print_summary()`) per CODEWIKI-009. All call sites (`setup_test_environment`, `create_config`, `validate_paths`, `execute_dependency_parser`, `verify_namespaces`, `_verify_namespace_counts`, `verify_cross_path_dependencies`, `verify_no_warnings`, `verify_file_counts`) now call `self.results.add_test(...)` instead of `self._assert(...)`. The old `print_validation_summary` method was removed and its logic folded into `TestResults.print_summary()`, which `run()` now calls directly; `run()` exits non-zero via `self.results.all_passed()`, preserving the crash-handling and cleanup behavior. Risk: this is a moderately invasive internal restructuring within one file (no shared/imported `TestResults` module was available to reuse, so a local class was defined instead) — if other scripts in the repo import a canonical `TestResults` from a shared module, this local duplicate may not match its exact API/signature exactly and should ideally be replaced with an import once that shared module's location is confirmed.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer

"passed": [],
"failed": [],
"warnings": []
}
self.results = TestResults()

def setup_test_environment(self) -> None:
"""Create test directory structure with sample files"""
Expand Down Expand Up @@ -75,7 +128,7 @@ def setup_test_environment(self) -> None:
# Create sample files in vendor/
self._create_vendor_files()

self._assert("Test directories created", True)
self.results.add_test("Test directories created", True)

def _create_main_files(self) -> None:
"""Create sample Python files in main/ directory"""
Expand Down Expand Up @@ -273,7 +326,7 @@ def create_config(self) -> None:
for i, path in enumerate(self.config.additional_source_paths, 1):
print(f" {i}. {path}")

self._assert("Config created with 3 source paths", True)
self.results.add_test("Config created with 3 source paths", True)

def validate_paths(self) -> None:
"""Validate all configured paths exist"""
Expand All @@ -292,7 +345,7 @@ def validate_paths(self) -> None:
print(f"{'✓' if exists else '✗'} Additional path exists: {path}")
all_valid = all_valid and exists

self._assert("All paths validated successfully", all_valid)
self.results.add_test("All paths validated successfully", all_valid)

def execute_dependency_parser(self) -> None:
"""Run DependencyGraphBuilder with all configured paths"""
Expand All @@ -305,7 +358,7 @@ def execute_dependency_parser(self) -> None:
# Check if multi-path mode was detected
is_multi = self.config.is_multi_path_mode()
print(f"{'✓' if is_multi else '✗'} Multi-path mode detected: {is_multi}")
self._assert("Multi-path mode detected", is_multi)
self.results.add_test("Multi-path mode detected", is_multi)

# Build dependency graph
print("\nBuilding dependency graph...")
Expand Down Expand Up @@ -345,14 +398,14 @@ def verify_namespaces(self) -> None:

if missing:
print(f"\n✗ Missing namespaces: {missing}")
self._assert("All expected namespaces present", False)
self.results.add_test("All expected namespaces present", False)
else:
print(f"\n✓ All expected namespaces present: {expected}")
self._assert("All expected namespaces present", True)
self.results.add_test("All expected namespaces present", True)

if unexpected:
print(f"⚠ Unexpected namespaces: {unexpected}")
self.results["warnings"].append(f"Unexpected namespaces: {unexpected}")
self.results.add_test("No unexpected namespaces", True, f"Unexpected namespaces: {unexpected}")

# Verify component counts
self._verify_namespace_counts(namespaces)
Expand All @@ -379,7 +432,7 @@ def _verify_namespace_counts(self, namespaces: Dict[str, List[str]]) -> None:
print(f" Found components: {sorted(namespaces.get(namespace, []))}")
all_match = all_match and match

self._assert("Component counts match expectations", all_match)
self.results.add_test("Component counts match expectations", all_match)

def verify_cross_path_dependencies(self) -> None:
"""Verify cross-path dependencies are correctly resolved"""
Expand Down Expand Up @@ -413,11 +466,11 @@ def verify_cross_path_dependencies(self) -> None:
# but not resolved to specific components across namespaces
if len(cross_deps) > 0:
print(f"\n✓ Cross-path dependencies detected: {len(cross_deps)} found")
self._assert("Cross-path dependencies detected", True)
self.results.add_test("Cross-path dependencies detected", True)
else:
print(f"\n✓ No cross-path dependencies detected (expected - not implemented in AST parser yet)")
print(f" Note: Import statements are parsed but not resolved across namespaces")
self._assert("Multi-path mode working (dependencies optional)", True)
self.results.add_test("Multi-path mode working (dependencies optional)", True)

def verify_no_warnings(self) -> None:
"""Verify no 'not found' warnings were generated"""
Expand All @@ -430,7 +483,7 @@ def verify_no_warnings(self) -> None:

if not has_warnings:
print("✓ No warning tracking mechanism (expected behavior)")
self._assert("No warnings generated", True)
self.results.add_test("No warnings generated", True)
return

warnings = getattr(self.builder, 'warnings', [])
Expand All @@ -439,10 +492,10 @@ def verify_no_warnings(self) -> None:
print(f"⚠ Found {len(warnings)} warnings:")
for warning in warnings:
print(f" - {warning}")
self._assert("No warnings generated", False)
self.results.add_test("No warnings generated", False)
else:
print("✓ No warnings generated")
self._assert("No warnings generated", True)
self.results.add_test("No warnings generated", True)

def verify_file_counts(self) -> None:
"""Verify expected number of components were analyzed"""
Expand All @@ -469,7 +522,7 @@ def verify_file_counts(self) -> None:
print(f"Expected: {expected_total}")

match = total == expected_total
self._assert("Component counts match expectations", match)
self.results.add_test("Component counts match expectations", match)

def print_detailed_output(self) -> None:
"""Print comprehensive analysis output"""
Expand Down Expand Up @@ -509,54 +562,12 @@ def print_detailed_output(self) -> None:
for namespace, count in sorted(namespaces.items()):
print(f" {namespace}: {count} components")

def print_validation_summary(self) -> None:
"""Print final validation summary"""
print("\n" + "="*80)
print("VALIDATION SUMMARY")
print("="*80)

total = len(self.results["passed"]) + len(self.results["failed"])
passed = len(self.results["passed"])
failed = len(self.results["failed"])
warnings = len(self.results["warnings"])

print(f"\nTotal Assertions: {total}")
print(f" ✓ Passed: {passed}")
if failed > 0:
print(f" ✗ Failed: {failed}")
if warnings > 0:
print(f" ⚠ Warnings: {warnings}")

if failed > 0:
print("\nFailed Assertions:")
for msg in self.results["failed"]:
print(f" ✗ {msg}")

if warnings > 0:
print("\nWarnings:")
for msg in self.results["warnings"]:
print(f" ⚠ {msg}")

print("\n" + "="*80)
if failed == 0:
print("✅ INTEGRATION TEST PASSED")
else:
print("❌ INTEGRATION TEST FAILED")
print("="*80)

def cleanup(self) -> None:
"""Clean up test directory"""
if self.test_dir and self.test_dir.exists():
shutil.rmtree(self.test_dir)
print(f"\n🧹 Cleaned up test directory: {self.test_dir}")

def _assert(self, message: str, condition: bool) -> None:
"""Record assertion result"""
if condition:
self.results["passed"].append(message)
else:
self.results["failed"].append(message)

def run(self) -> int:
"""Execute complete integration test"""
try:
Expand All @@ -569,9 +580,9 @@ def run(self) -> int:
self.verify_no_warnings()
self.verify_file_counts()
self.print_detailed_output()
self.print_validation_summary()
self.results.print_summary()

return 0 if len(self.results["failed"]) == 0 else 1
return 0 if self.results.all_passed() else 1

except Exception as e:
print(f"\n❌ INTEGRATION TEST CRASHED: {e}")
Expand Down
69 changes: 46 additions & 23 deletions test-multi-path/test_multi_path.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,48 @@ class Colors:
END = '\033[0m'


class TestResults:
"""Accumulates test results across the suite and prints a summary."""

def __init__(self):
self.results = {}

def add_test(self, name: str, passed: bool):
"""Record the result of a single test.

Args:
name: Test name
passed: Whether the test passed
"""
self.results[name] = passed

def print_summary(self) -> int:
"""Print formatted summary of all recorded test results.

Returns:
Exit code: 0 if all tests passed, 1 otherwise
"""
print_header("Test Results Summary")

passed = sum(1 for r in self.results.values() if r)
total = len(self.results)

for test_name, result in self.results.items():
if result:
print_success(f"{test_name}")
else:
print_error(f"{test_name}")

print(f"\n{Colors.BOLD}Total: {passed}/{total} tests passed{Colors.END}")

if passed == total:
print(f"{Colors.GREEN}{Colors.BOLD}✓ All tests passed!{Colors.END}\n")
return 0
else:
print(f"{Colors.RED}{Colors.BOLD}✗ Some tests failed{Colors.END}\n")
return 1


def print_header(text: str):
"""Print formatted test header."""
print(f"\n{Colors.BOLD}{Colors.BLUE}{'='*70}{Colors.END}")
Comment on lines 42 to 89

@flamingo flamingo Bot Sep 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 test_multi_path.py defines test_ functions returning bool but never assembles them via a TestResults accumulator*

Added a TestResults class (with add_test and print_summary methods) near the top of test-multi-path/test_multi_path.py, and changed run_all_tests() to instantiate TestResults(), call test_results.add_test(test_name, result) in place of the old results[test_name] = result dict assignments, and return test_results.print_summary() instead of the inline summary-printing/exit-code logic that previously lived directly in run_all_tests. The individual test_* functions (test_single_path, test_multiple_paths, etc.) are unchanged in behavior/output — they still return bool and print ad hoc colored messages — but their results are now assembled through the mandated TestResults accumulator rather than a raw dict, satisfying CODEWIKI-009's pattern. Risk: I could not see the exact CODEWIKI-009 spec for TestResults, so the method signatures (add_test(name, passed), print_summary() -> int) are a reasonable but unverified guess at the mandated interface; a stricter conforming implementation might require additional fields (e.g., timing, error messages) or a different call signature.

🤖 Prompt for AI agents 
In test-multi-path/test_multi_path.py around line 106, review and complete this code-review fix: test_multi_path.py defines test_* functions returning bool but never assembles them via a TestResults accumulator.
What the draft fix changed: Added a `TestResults` class (with `add_test` and `print_summary` methods) near the top of `test-multi-path/test_multi_path.py`, and changed `run_all_tests()` to instantiate `TestResults()`, call `test_results.add_test(test_name, result)` in place of the old `results[test_name] = result` dict assignments, and return `test_results.print_summary()` instead of the inline summary-printing/exit-code logic that previously lived directly in `run_all_tests`. The individual `test_*` functions (`test_single_path`, `test_multiple_paths`, etc.) are unchanged in behavior/output — they still return bool and print ad hoc colored messages — but their results are now assembled through the mandated `TestResults` accumulator rather than a raw dict, satisfying CODEWIKI-009's pattern. Risk: I could not see the exact CODEWIKI-009 spec for `TestResults`, so the method signatures (`add_test(name, passed)`, `print_summary() -> int`) are a reasonable but unverified guess at the mandated interface; a stricter conforming implementation might require additional fields (e.g., timing, error messages) or a different call signature.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -485,38 +527,19 @@ def run_all_tests():
("Relative vs Absolute Paths", test_relative_vs_absolute_paths),
]

results = {}
test_results = TestResults()

for test_name, test_func in tests:
try:
result = test_func()
results[test_name] = result
test_results.add_test(test_name, result)
except Exception as e:
print_error(f"Test '{test_name}' crashed: {str(e)}")
import traceback
traceback.print_exc()
results[test_name] = False

# Print summary
print_header("Test Results Summary")

passed = sum(1 for r in results.values() if r)
total = len(results)

for test_name, result in results.items():
if result:
print_success(f"{test_name}")
else:
print_error(f"{test_name}")

print(f"\n{Colors.BOLD}Total: {passed}/{total} tests passed{Colors.END}")
test_results.add_test(test_name, False)

if passed == total:
print(f"{Colors.GREEN}{Colors.BOLD}✓ All tests passed!{Colors.END}\n")
return 0
else:
print(f"{Colors.RED}{Colors.BOLD}✗ Some tests failed{Colors.END}\n")
return 1
return test_results.print_summary()


if __name__ == "__main__":
Expand Down
Loading