test: wire orphaned core integration tests into fastskill-core#196
Merged
Conversation
The workspace-root `tests/` tree was orphaned by the crate split: the root is a virtual manifest, so its `tests/` is never compiled, and the files still import the pre-split crate name `fastskill::`. Result: a whole suite that looked like coverage but never ran. Move the pure-core, currently-valid tests into `crates/fastskill-core/tests/` (so Cargo compiles them) and rename `fastskill::` -> `fastskill_core::`. 10 files / 60 tests now actually run in CI (both the default and --all-features jobs), including index_manager/registry_atomic/registry_index — which directly exercise the atomic-update lock/temp fix from #195. Bonus: registry_atomic_test had two tests `#[ignore]`'d for the exact bugs that #194/#195 fixed — "concurrent writes to same skill not properly serialized" and "data loss in sequential operations (only 1 of 100 entries saved)". Both pass now, so they're un-ignored and become real regression coverage for the fix. The timing-sensitive perf SLA test (SC-003, p95<100ms) is `#[ignore]`'d instead — it flakes under parallel nextest and isn't a correctness check. Deferred (still orphaned, separate follow-up): - core_manifest_tests — real drift (FastSkillToolConfig gained `auto_reindex`). - tests/http/** and tests/unit/** — multi-file module trees needing an entry. - The CLI e2e suite (tests/cli/**, tests/cli_tests.rs) and process/wiremock tests — need binary wiring, snapshot regen, and command-surface drift fixes. fmt clean; clippy clean on default and --all-features; all wired tests green on both feature configs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The workspace-root
tests/tree was orphaned by the crate split. The root is a virtual manifest, so itstests/is never compiled, and the files still import the pre-split crate namefastskill::. The result was a large suite that looked like coverage but never ran — including the tests that should guard the registry atomic-update path we just fixed in #195.Change
Move the pure-core, currently-valid tests into
crates/fastskill-core/tests/(so Cargo compiles them) and renamefastskill::→fastskill_core::.10 files / 60 tests now actually run in CI (both the default and
--all-featuresjobs):index_manager_test,registry_atomic_test,registry_index_test,core_project_tests,cors_tests,git_consumption_test,integration_skill_project_toml_test,integration_unified_format_test,metadata_validation_test,skill_project_toml_test.These directly exercise the
atomic_updatelock/temp fix from #195 (e.g.test_concurrent_writes_different_skills).Bonus — two ignored tests that #194/#195 actually fixed
registry_atomic_testhad two tests#[ignore]'d with TODOs describing the exact bugs those PRs fixed:Both pass now — un-ignored, so they become real regression coverage. The timing-sensitive perf SLA test (
test_index_update_performance, SC-003 p95<100ms) is#[ignore]'d instead: it flakes under parallel nextest and isn't a correctness check.Deferred (still orphaned — separate follow-ups)
core_manifest_tests— real API drift (FastSkillToolConfiggainedauto_reindex); needs a test update.tests/http/**andtests/unit/**— multi-file module trees that need an entry-point module to wire.tests/cli/**,tests/cli_tests.rs) and the process/wiremock tests — need binary wiring, snapshot regeneration, and command-surface drift fixes (removedshow/disable/sync, URL rename, etc.).Verification
fmt clean; clippy clean on default and
--all-features; all wired tests green on both feature configs; pre-push nextest gate passes.🤖 Generated with Claude Code