Skip to content

Add unittest test suite for sync engine#127

Merged
sebastientaggart merged 3 commits into
devfrom
feature/sync-engine-tests
Apr 12, 2026
Merged

Add unittest test suite for sync engine#127
sebastientaggart merged 3 commits into
devfrom
feature/sync-engine-tests

Conversation

@sebastientaggart

Copy link
Copy Markdown
Member

Add a comprehensive unittest test suite for the sync engine (sync.py) with 83 tests covering YAML parsing, conditional blocks, placeholder substitution, hash-based change detection, adapter loading, full sync_skill flow (including dry-run, idempotency, customization protection, and --force), validation, CLI integration, and golden-file snapshot regression tests.

Also adds:

  • make test target
  • CI workflow (.github/workflows/test.yml) running tests on Python 3.9 + 3.12 with Codecov coverage upload
  • Tests and Codecov badges in README
  • __pycache__/ and *.pyc to .gitignore

Issue #121

@sebastientaggart

Copy link
Copy Markdown
Member Author

Review Summary

Verdict: APPROVE

Findings

  • [NOTE] TestGoldenFileSnapshots.test_all_generated_files_are_current duplicates the sync logic inline (lines 904-918 of the diff) rather than calling sync_skill with dry_run=True -- this means if the sync engine's rendering pipeline changes (e.g. a new frontmatter key or processing step), the golden test must be updated in lockstep or it will silently diverge. Acceptable for now since the CI dry-run check is the primary guard.

  • [NOTE] Several CLI integration tests (test_validate_with_real_config, test_dry_run_with_real_config, test_skill_filter) mutate os.getcwd() via os.chdir inside the test. A failure between chdir and the finally block could leave the process in an unexpected directory for subsequent tests. The finally blocks mitigate this, but tempfile.TemporaryDirectory as a context manager plus monkeypatching Path.cwd would be more robust. Not blocking.

No correctness errors, no secrets, no .env or build artifacts committed. Commit messages are in imperative mood. The test helpers, assertions, and edge-case coverage (customization guard, legacy marker migration, conditional blocks, --force override) all align correctly with the sync.py implementation.

@sebastientaggart

Copy link
Copy Markdown
Member Author

Review Summary

Verdict: REQUEST CHANGES

Reviewing the follow-up commit e61d21b2 ("Address review findings in test suite").

Findings

  • [CRITICAL] CI workflow fails: coverage package not installed. The test.yml workflow uses python3 -m coverage run ... but never installs the coverage package (pip install coverage). The Python 3.12 job fails with No module named coverage (confirmed via the failed run logs). Add a step before the coverage run:

    - name: Install coverage
      if: matrix.python-version == '3.12'
      run: pip install coverage
  • [NOTE] The refactored TestMainCLI using shared setUp/tearDown for os.chdir safety is a clean improvement. The test_no_adapters_exits_1 test correctly relies on tearDown to restore cwd even though it calls os.chdir(tmpdir) directly rather than via _chdir_to_project().

  • [NOTE] The golden-file test now delegates rendering to sync_skill instead of duplicating the pipeline inline -- good fix. The temp directory cleanup via shutil.rmtree in a finally block is correct.

@sebastientaggart

Copy link
Copy Markdown
Member Author

Review Summary

Verdict: APPROVE

Findings

  • [NOTE] In .github/workflows/test.yml, tests run twice on the Python 3.12 matrix leg: once in the "Run tests" step (plain unittest discover) and again in "Run tests with coverage" (coverage run). The plain run could be skipped for 3.12 by adding if: matrix.python-version != '3.12' to the first step, but this is cosmetic -- CI results are correct either way.

  • [NOTE] coverage.xml (generated during CI) is not in .gitignore. Low risk since it only appears in CI runners, but worth adding alongside the __pycache__/ entry if someone ever runs coverage locally.

No critical or blocking issues. The test suite is thorough (83 tests covering YAML parsing, conditionals, placeholders, hashing, adapter loading, sync flow, validation, CLI integration, and golden-file regression), well-structured with proper temp directory cleanup, and follows project conventions.

@sebastientaggart
sebastientaggart merged commit c34838c into dev Apr 12, 2026
6 checks passed
@sebastientaggart
sebastientaggart deleted the feature/sync-engine-tests branch April 12, 2026 02:41
This was referenced Apr 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant