refactor: restructure tests to unit/integration and add dbt dependency

[pgoell]

• Reorganize test directory structure: test/ -> tests/unit/ and tests/integration/
• Move existing tests to tests/unit/ as they use mocking
• Add dbt-core and dbt-duckdb as dev dependencies for integration testing
• Create shared conftest.py with common fixtures
• Add integration tests that run actual dbt commands
• Update pyproject.toml with pytest markers, separate test tasks
• Update ruff and ty configurations for new tests/ path

[claude]

Pull Request Review: Test Restructuring and Integration Tests

Overview

This PR successfully reorganizes the test structure and adds integration tests for dbt functionality. The changes are well-executed with good separation of concerns between unit and integration tests.

---

✅ Strengths

1. Clear Test Organization: The new tests/unit/ and tests/integration/ structure provides excellent separation, making it easy to run fast unit tests vs slower integration tests.

2. Proper Fixture Management: The shared conftest.py centralizes common fixtures (reset_logger_fixture, temp_cache_dir), eliminating duplication from individual test files.

3. Good Integration Test Design: The dbt integration tests use a minimal, isolated test project with DuckDB in-memory mode, avoiding external dependencies.

4. Configuration Updates: All related configurations (ruff, ty, pytest) were properly updated to reflect the new tests/ path.

5. Pytest Markers: The integration marker allows selective test execution (pytest -m "not integration"), which is essential for fast CI feedback loops.

---

🔍 Issues & Recommendations

1. CI Will Run Integration Tests Without Intention

Severity: Medium

The current .github/workflows/test.yml runs uv run pytest --cov, which will execute ALL tests including slow integration tests. This will:

• Significantly slow down CI runs
• Add dbt-core and dbt-duckdb dependencies to CI environment
• Potentially cause instability if dbt has issues

Recommendation: Update the CI workflow to explicitly exclude integration tests:

- name: Run tests
  run: uv run pytest -m "not integration" --cov --cov-report=xml

Or add a separate integration test job that runs on a schedule or manually.

2. Missing Cleanup in dbt_project Fixture

Severity: Low

The dbt_project fixture in test_dbt_integration.py:20-24 copies the fixture directory to tmp_path but doesn't explicitly clean up dbt's generated files (like target/ directory, dbt.log, etc.).

While pytest's tmp_path is automatically cleaned up, it's cleaner to explicitly handle dbt artifacts:

@pytest.fixture
def dbt_project(tmp_path):
    """Copy the dbt project fixture to a temporary directory."""
    project_dir = tmp_path / "dbt_project"
    shutil.copytree(DBT_PROJECT_DIR, project_dir)
    yield project_dir
    # Cleanup is automatic via tmp_path, but could be explicit for clarity

Consider adding autouse=True teardown in conftest if needed.

3. Integration Tests Could Be More Comprehensive

Severity: Low / Suggestion

Current integration tests verify basic dbt commands work, but could test:

• Model compilation and output validation
• Error handling (e.g., invalid model SQL)
• Log output verification
• Exit codes for various failure scenarios

Example:

def test_dbt_run_creates_expected_output(self, dbt_project):
    """Verify dbt run actually creates the model."""
    exit_code = run_dbt(
        ["run", "--project-dir", str(dbt_project), "--profiles-dir", str(dbt_project)]
    )
    assert exit_code == 0
    # Could verify target/ contains compiled SQL or check DuckDB for table

4. Dependency Version Constraints

Severity: Low

Adding dbt-core>=1.9.0 and dbt-duckdb>=1.9.0 is good, but consider:

• dbt-core 1.9.0 is quite recent (released ~Nov 2024)
• No upper bound means future breaking changes could break tests
• Consider dbt-core>=1.9.0,<2.0 for stability

5. Test Isolation: Module-Level Constants

Severity: Very Low

In test_dbt_integration.py:13-16, module-level constants reference file paths:

FIXTURES_DIR = Path(__file__).parent.parent / "fixtures"
DBT_PROJECT_DIR = FIXTURES_DIR / "dbt_project"

This is fine, but if tests are run from different working directories, it could theoretically cause issues. Using __file__ is correct here, so this is mostly a note.

---

🔒 Security Considerations

No security concerns identified. The integration tests:

• Use in-memory DuckDB (:memory:) - no persistent data
• Run in isolated temp directories
• Don't expose credentials or sensitive data
• Properly use subprocess through the existing run_dbt() function which already has safety measures

---

🎯 Performance Considerations

1. Integration tests will add ~5-10 seconds per test due to dbt initialization overhead. With 6 integration tests, this could add 30-60 seconds to test suite.

2. Large dependency footprint: dbt-core brings many transitive dependencies (see the massive uv.lock diff with 1,394 additions). Consider:

   • Optional dependency group: [dependency-groups.integration] separate from dev
   • Document that integration tests are optional for contributors

3. Parallel test execution: The integration tests should be safe to run in parallel since each uses isolated tmp_path directories.

---

📋 Test Coverage

Excellent addition to test coverage. The integration tests verify:

• ✅ dbt CLI passthrough works end-to-end
• ✅ Both direct function calls and CLI invocation paths
• ✅ Actual dbt parsing, compilation, and execution
• ✅ Integration with dbt-duckdb adapter

This complements the existing unit tests which verify argument passing and error handling with mocks.

---

📝 Documentation & Code Style

1. Docstrings: All new test methods have clear docstrings ✅
2. Type hints: Correctly omitted per CLAUDE.md (tests/* excludes ANN rules) ✅
3. Code formatting: Follows project conventions ✅
4. Comments: The note in test_brix_dbt_version() about subprocess output is helpful ✅

---

✅ Final Verdict

Approve with minor suggestions.

This is a solid refactoring that improves test organization and adds valuable integration testing. The main actionable item is updating CI to exclude integration tests by default to prevent slowdowns.

Recommended Actions Before Merge:

1. Required: Update .github/workflows/test.yml to exclude integration tests from default runs
2. Recommended: Add upper bound to dbt-core version constraint
3. Optional: Add a separate CI job for integration tests (scheduled or manual)

Post-Merge Considerations:

• Document in README/CONTRIBUTING how to run integration tests
• Monitor CI performance after merge
• Consider making integration deps optional with [dependency-groups.integration]

Great work on the test infrastructure improvements! 🎉

[claude]

Pull Request Review: Test Restructure and dbt Integration

Summary

This PR successfully restructures the test directory from test/ to tests/ with clear separation into unit/ and integration/ subdirectories. It adds dbt-core and dbt-duckdb as dev dependencies and introduces comprehensive integration tests for dbt passthrough functionality.

✅ Strengths

Code Quality

• Well-organized structure: Clear separation between unit and integration tests follows Python best practices
• Comprehensive test coverage: Integration tests cover key dbt operations (version, debug, parse, run)
• Proper test isolation: Using tmp_path fixture to copy dbt project ensures test isolation
• Good documentation: Test docstrings clearly explain what each test validates
• Consistent style: Code follows the project's Google-style docstring convention

Architecture

• Smart fixture design: The shared conftest.py reduces code duplication by centralizing common fixtures
• Proper test markers: Using @pytest.mark.integration enables selective test execution
• Minimal test fixtures: The dbt project fixture is simple and focused (single model, DuckDB in-memory)

Configuration

• Clean pyproject.toml updates: New poe tasks (test-unit, test-integration) provide granular test execution
• Correct path updates: Ruff and ty configurations properly updated for new tests/ path
• Appropriate dependency versions: Using dbt-core>=1.9.0 and dbt-duckdb>=1.9.0 is current and reasonable

🔍 Areas for Improvement

1. Integration Test Isolation (Minor)

Location: tests/integration/test_dbt_integration.py:31-34

The test_dbt_version test doesn't use the isolated dbt_project fixture, which means it could be affected by user's local dbt configuration:

def test_dbt_version(self):
    """Test that dbt --version runs successfully."""
    exit_code = run_dbt(["--version"])
    assert exit_code == 0

Recommendation: Consider adding a comment explaining that this test intentionally uses global dbt, or make it more isolated by setting DBT_PROFILES_DIR environment variable.

2. Duplicate Test Classes (Minor)

Location: tests/integration/test_dbt_integration.py:28-76

Both TestDbtIntegration and TestDbtCliIntegration classes test similar functionality - the difference is only whether they call run_dbt() directly or via the CLI. This creates some duplication.

Recommendation: This is acceptable for now as it tests different code paths (direct function vs CLI entry point), but consider if all these duplicated tests are necessary. Perhaps some could be parametrized.

3. Missing Cleanup Verification (Minor)

Location: tests/integration/test_dbt_integration.py:19-24

The dbt_project fixture copies the project to a temporary directory but doesn't verify cleanup or check for leftover artifacts.

Recommendation: Add a test that verifies dbt creates expected artifacts in the right location:

def test_dbt_run_creates_target_dir(self, dbt_project):
    run_dbt(["run", "--project-dir", str(dbt_project), "--profiles-dir", str(dbt_project)])
    assert (dbt_project / "target").exists()
    # tmp_path automatically cleans up

4. Example Model Could Be More Realistic (Cosmetic)

Location: tests/fixtures/dbt_project/models/example_model.sql:1

The current model is ultra-minimal:

select 1 as id, 'test' as name

Recommendation: While this works fine, consider adding a slightly more realistic example that tests dbt's Jinja templating or ref() functions to ensure those features work in the passthrough. Example:

-- This tests that dbt's Jinja engine works
select 
    1 as id, 
    'test' as name,
    '{{ "compiled" }}' as templated_value

5. Error Case Testing Missing (Enhancement)

Location: tests/integration/test_dbt_integration.py

The integration tests only verify happy paths (exit_code == 0). There are no tests for error conditions.

Recommendation: Consider adding at least one test that verifies error handling:

def test_dbt_run_with_invalid_project(self, tmp_path):
    """Test that dbt fails gracefully with invalid project."""
    invalid_dir = tmp_path / "invalid"
    invalid_dir.mkdir()
    exit_code = run_dbt(["run", "--project-dir", str(invalid_dir)])
    assert exit_code != 0  # Should fail, not crash

🔒 Security

No security concerns identified. The test changes don't introduce any security vulnerabilities.

⚡ Performance

Dependency Size: Adding dbt-core and dbt-duckdb significantly increases the dependency footprint (~1500 lines in uv.lock). This is expected for integration testing but means:

• Longer dev environment setup time
• Larger Docker images if tests run in CI
• This is acceptable as these are dev dependencies only

Test Execution: Integration tests will be slower than unit tests due to actual dbt execution. The pytest marker system allows skipping them when needed (pytest -m "not integration").

📋 Test Coverage

Excellent coverage of the dbt passthrough functionality:

• ✅ Direct function calls tested
• ✅ CLI integration tested
• ✅ Version, debug, parse, and run commands tested
• ✅ Unit tests verify mocking and argument passing
• ✅ Integration tests verify actual dbt execution

🎯 Recommendations Priority

1. High: None - the PR is solid as-is
2. Medium: Consider adding error case testing (item fix: change code review workflow to trigger on /review command #5)
3. Low: Add cleanup verification test (item Feat/version checking #3), improve example model (item refactor: restructure tests to unit/integration and add dbt dependency #4)
4. Nice-to-have: Add isolation comment for version test (item Add Claude Code GitHub Workflow #1), consider parametrizing duplicate tests (item dbt token authentication commands #2)

✅ Final Verdict

APPROVED - This is a high-quality refactor that follows best practices. All tests pass, the code is well-organized, and the separation of concerns is clear. The additions are valuable and the migration from test/ to tests/ aligns with Python community standards.

The minor suggestions above are enhancements rather than blockers. The PR is ready to merge as-is.

---

Test Results: ✅ pre-commit: SUCCESS | ✅ test: SUCCESS

Great work on maintaining test quality while expanding coverage! 🎉

[claude]

Pull Request Review

This PR successfully restructures the test suite with a clear separation between unit and integration tests. Overall, this is a well-executed refactoring that improves test organization and adds valuable integration coverage.

✅ Strengths

Test Organization

• Clean separation of concerns: tests/unit/ for mocked tests, tests/integration/ for actual dbt execution
• Proper pytest markers (@pytest.mark.integration) allowing selective test execution
• Shared fixtures moved to tests/conftest.py following pytest best practices
• New poe tasks (test-unit, test-integration) provide fine-grained test control

Integration Tests

• Comprehensive coverage of key dbt commands: --version, debug, parse, run
• Tests both direct function calls (run_dbt()) and CLI invocation (brix dbt)
• Uses DuckDB with in-memory database - excellent choice for fast, dependency-free testing
• Fixture pattern with shutil.copytree ensures test isolation

Configuration

• Documentation updated appropriately in CLAUDE.md
• Ruff and ty configurations updated for new paths
• Dependencies (dbt-core, dbt-duckdb) properly added as dev dependencies

🔍 Issues & Recommendations

1. Security: Subprocess Shell Injection Risk ⚠️

Location: src/brix/modules/dbt/passthrough.py:63

While the comment acknowledges this is intentional, the current implementation has a potential security issue:

result = subprocess.run([dbt_path, *args])  # noqa: S603

Issue: If find_dbt_executable() is modified in the future to accept user input or path discovery logic, this could become a security vulnerability. The noqa: S603 suppresses the security warning.

Recommendation: Add validation or documentation about the security contract:

# SECURITY: dbt_path must be validated before this point.
# Only use trusted paths from find_dbt_executable().
result = subprocess.run([dbt_path, *args], check=False)  # noqa: S603

2. Missing Error Handling in Integration Tests

Location: tests/integration/test_dbt_integration.py

The integration tests only check for exit_code == 0 but don't verify:

• Output content (e.g., that models actually ran)
• Generated artifacts (e.g., target/ directory contents)
• Error messages for negative test cases

Recommendation: Consider adding at least one test that verifies actual dbt execution results:

def test_dbt_run_creates_artifacts(self, dbt_project):
    """Verify dbt run creates expected artifacts."""
    exit_code = run_dbt(["run", "--project-dir", str(dbt_project), "--profiles-dir", str(dbt_project)])
    assert exit_code == 0
    assert (dbt_project / "target" / "manifest.json").exists()

3. Test Fixture Could Be More Robust

Location: tests/fixtures/dbt_project/models/example_model.sql:1

The example model is minimal (just select 1 as id, 'test' as name), which is fine for basic tests but doesn't cover:

• Jinja templating
• Model dependencies (refs)
• Different materialization types

Not blocking, but consider adding a second model for more comprehensive testing in the future.

4. Conftest Fixture Naming

Location: tests/conftest.py:10

The fixture reset_logger_fixture has redundant naming (_fixture suffix). Pytest fixtures don't need this suffix.

Minor suggestion: Rename to reset_logger or auto_reset_logger for clarity:

@pytest.fixture(autouse=True)
def auto_reset_logger():
    """Reset logger before and after each test."""

5. Missing Test for Integration Marker

Location: pyproject.toml:112-113

The pytest configuration adds an integration marker, but there's no test verifying that pytest -m "not integration" actually skips the integration tests.

Recommendation: Verify locally that:

uv run poe test-unit  # Should skip integration tests
uv run pytest -m "not integration"  # Should also skip integration tests

📊 Test Coverage Considerations

The PR doesn't show updates to coverage configuration. Consider:

• Should integration tests be included in coverage reports?
• Are coverage thresholds still appropriate with the new test structure?

🎯 Performance

Integration tests will be slower than unit tests. Good practices observed:

• ✅ Using in-memory DuckDB database
• ✅ Separate poe tasks allow running fast unit tests during development
• ✅ Markers allow CI to run suites independently

Summary

This is a high-quality refactoring that improves the project's test infrastructure. The only significant concern is the subprocess security pattern in passthrough.py, which should have clearer documentation about the security contract. The integration tests provide valuable coverage but could benefit from deeper assertions about dbt execution results.

Recommendation: ✅ Approve with minor suggestions

The current implementation is solid for merging. The suggestions above are mostly for future enhancements and better security documentation.