Document Chekov 0.1 behavior before the 0.2 refactor - #29
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The CI workflow change removes the SonarCloud/Clippy job without being called out, which may unintentionally alter required CI checks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR establishes a reproducible Chekov 0.1 behavioral baseline ahead of the 0.2 refactor by pinning a compatible Actix macro crate version, adding a regression test for failed-command persistence behavior, and documenting current runtime/persistence/replay semantics.
Changes:
- Pin
actix_deriveto=0.6.0via workspace deps and consume it in relevant crates to keep Actix 0.12 macro expansion compiling. - Add an in-memory test asserting a failed command does not mutate aggregate state/version and does not create a stream.
- Add a detailed “0.1 behavioral baseline” document capturing current execution, persistence, replay, subscription, and concurrency behavior (and known gaps).
File summaries
| File | Description |
|---|---|
| docs/chekov-0.1-behavioral-baseline.md | New baseline document for 0.1 behavior and known gaps/required 0.2 decisions. |
| crates/event_store/Cargo.toml | Adds actix_derive as a workspace dependency to stabilize Actix macro compilation. |
| crates/chekov/src/tests/aggregates/persistency.rs | Adds a regression test for failed-command “no state/version/stream change” behavior. |
| crates/chekov/Cargo.toml | Adds actix_derive workspace dependency for Actix 0.12 macro compatibility. |
| Cargo.toml | Pins actix_derive to =0.6.0 in [workspace.dependencies]. |
| .gitignore | Ignores .worktrees/ directory. |
| .github/workflows/ci.yml | Updates GitHub Actions tooling and switches coverage generation to cargo llvm-cov (also removes SonarCloud job). |
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The Coveralls step references secrets.github_token instead of the standard secrets.GITHUB_TOKEN, which will typically break coverage uploads.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new baseline document contains at least one incorrect test reference and a brittle hard-coded test count that should be corrected to keep the baseline reliable.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
docs/chekov-0.1-behavioral-baseline.md:25
- This hard-coded passing test count is already inconsistent with the PR’s own verification notes (and will drift whenever tests are added/removed). Consider wording this without an exact number so the baseline doc stays accurate over time.
This issue also appears on line 39 of the same file.
At capture time, the suite reported 70 passing tests, one ignored test, and no failures. This count includes unit and documentation tests.
docs/chekov-0.1-behavioral-baseline.md:42
- This baseline-test reference doesn’t match the current test suite (there is no
failed_command_leaves_aggregate_and_stream_unchangedtest). Update the list to point at the actual persistency tests that cover the failed-command behavior.
- `tests::aggregates::runtime::can_execute_a_command`
- `tests::aggregates::runtime::can_recover_from_fail_execution`
- `tests::aggregates::persistency::failed_command_leaves_aggregate_and_stream_unchanged`
- `tests::aggregates::persistency::should_not_persist_events_when_command_returns_no_events`
- Files reviewed: 6/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The baseline documentation currently references a nonexistent test name (and includes a brittle test-count statement), which undermines the PR’s goal of providing an accurate, traceable behavioral baseline.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
docs/chekov-0.1-behavioral-baseline.md:42
- The baseline doc lists
tests::aggregates::persistency::failed_command_leaves_aggregate_and_stream_unchanged, but that test does not exist in the codebase; the current tests arefailed_command_does_not_create_streamandfailed_command_leaves_existing_aggregate_and_stream_unchanged. Update the references so the doc stays traceable to real tests.
- `tests::aggregates::runtime::can_execute_a_command`
- `tests::aggregates::runtime::can_recover_from_fail_execution`
- `tests::aggregates::persistency::failed_command_leaves_aggregate_and_stream_unchanged`
- `tests::aggregates::persistency::should_not_persist_events_when_command_returns_no_events`
- Files reviewed: 6/7 changed files
- Comments generated: 2
- Review effort level: Lite
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
Done in the latest commit:
|
There was a problem hiding this comment.
🟡 Changes recommended
The check job in .github/workflows/ci.yml has invalid YAML/step structure so cargo check --all will not run as intended.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
docs/chekov-0.1-behavioral-baseline.md:43
- This section describes the “failed first command leaves the stream nonexistent” behavior, but the baseline test list omits the test that directly covers that case (
failed_command_does_not_create_stream). To keep the baseline documentation self-consistent, include that test in the list.
Baseline tests:
- `tests::aggregates::runtime::can_execute_a_command`
- `tests::aggregates::runtime::can_recover_from_fail_execution`
- `tests::aggregates::persistency::failed_command_leaves_aggregate_and_stream_unchanged`
- `tests::aggregates::persistency::should_not_persist_events_when_command_returns_no_events`
- Files reviewed: 6/7 changed files
- Comments generated: 1
- Review effort level: Lite
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
Fixed in the latest commit: added the missing |
There was a problem hiding this comment.
🔵 Needs a closer look
The new baseline document references a non-existent test name, and the CI workflow includes repeated Postgres startup steps that appear unnecessary for the commands being run and add avoidable CI risk/cost.
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
docs/chekov-0.1-behavioral-baseline.md:42
- The baseline test list references
tests::aggregates::persistency::failed_command_leaves_aggregate_and_stream_unchanged, but that test does not exist in the repo. The persistency coverage for failed commands is currently infailed_command_does_not_create_streamandfailed_command_leaves_existing_aggregate_and_stream_unchanged, so the documentation should reference the actual test names to keep the baseline reproducible.
- `tests::aggregates::runtime::can_execute_a_command`
- `tests::aggregates::runtime::can_recover_from_fail_execution`
- `tests::aggregates::persistency::failed_command_leaves_aggregate_and_stream_unchanged`
- `tests::aggregates::persistency::should_not_persist_events_when_command_returns_no_events`
.github/workflows/ci.yml:20
- The Postgres container startup/wait in this job is unnecessary for
cargo check --all(no tests are executed and the workspace already usessqlxoffline mode). Keeping these steps adds CI time and can fail ifpg_isreadyis unavailable.
This issue also appears in the following locations of the same file:
- line 50
- line 68
- line 108
- run: docker compose -f scripts/tests/docker-compose.yml run -d -p 5432:5432 postgres
- run: until pg_isready -h localhost -p 5432; do sleep 1; done
- run: cargo check --all
.github/workflows/ci.yml:54
- The Postgres container startup/wait here appears unused by
cargo test --all(the repo’s baseline notes the default suite is database-independent). If the tests don’t require a live DB, removing these steps will reduce runtime and avoid relying onpg_isreadybeing present on the runner.
- run: docker compose -f scripts/tests/docker-compose.yml run -d -p 5432:5432 postgres
- run: until pg_isready -h localhost -p 5432; do sleep 1; done
- name: Run cargo test
run: cargo test --all
.github/workflows/ci.yml:71
- The Postgres container startup/wait in the doc build job adds time and potential flakiness, but
cargo docdoes not need a running database. Dropping these steps should make the job faster and more reliable.
- run: docker compose -f scripts/tests/docker-compose.yml run -d -p 5432:5432 postgres
- run: until pg_isready -h localhost -p 5432; do sleep 1; done
- run: cargo doc --no-deps --all --exclude bank
.github/workflows/ci.yml:112
- The coverage job starts Postgres and waits for it, but the subsequent command runs
cargo llvm-covagainst the workspace tests; if the test suite is DB-independent (as documented), these steps are unnecessary overhead and can fail ifpg_isreadyisn’t available on the runner.
- run: docker compose -f scripts/tests/docker-compose.yml run -d -p 5432:5432 postgres
- run: until pg_isready -h localhost -p 5432; do sleep 1; done
- name: Generate coverage report
run: cargo llvm-cov --all-features --workspace --exclude bank --exclude gift_shop --no-fail-fast --lcov --output-path lcov.info
- Files reviewed: 6/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
7091a20 to
e743b0d
Compare
Summary
Chekov 0.1 did not have a reproducible behavioral baseline for the 0.2 refactor. Fresh Cargo resolution also selected Actix derive releases that no longer compile with Actix 0.12.
This PR:
actix_deriveto the compatible 0.6.0 releaseCI changes
The existing workflow failed before reaching the repository checks because several actions and commands were obsolete. The updated workflow uses current checkout and artifact actions, Docker Compose v2, the stable Rust toolchain, and
cargo llvm-covfor coverage.SonarCloud analysis is removed, but Clippy remains as a standalone job and publishing waits for it.
sonar-project.propertiesremains in the repository. The Coveralls step uses the documentedsecrets.GITHUB_TOKENspelling.Verification
cargo check --allcargo test(71 passed, 1 ignored)cargo clippy --workspace --all-targets --all-featurescargo fmt --all -- --checkgit diff --checkLinear: FRE-32