|
| 1 | +# Add Coverage Check to Pre-commit Script |
| 2 | + |
| 3 | +**Issue**: #86 |
| 4 | +**Parent Epic**: #85 - Coverage & Reporting EPIC |
| 5 | +**Related**: [85-epic-coverage-and-reporting.md](./85-epic-coverage-and-reporting.md) |
| 6 | + |
| 7 | +## Overview |
| 8 | + |
| 9 | +Add code coverage validation to the pre-commit script (`scripts/pre-commit.sh`) as the last check in the steps array. The check uses the `cargo cov-check` alias which validates that coverage meets the 85% threshold and will fail the pre-commit if coverage drops below this level. |
| 10 | + |
| 11 | +## Goals |
| 12 | + |
| 13 | +- [ ] Make coverage validation part of the pre-commit process |
| 14 | +- [ ] Provide developers immediate feedback about coverage impact |
| 15 | +- [ ] Run coverage check last to ensure all other checks pass first (saves time) |
| 16 | +- [ ] Use existing step infrastructure for consistency |
| 17 | + |
| 18 | +## 🏗️ Architecture Requirements |
| 19 | + |
| 20 | +**DDD Layer**: Infrastructure (Tooling/Scripts) |
| 21 | +**Module Path**: `scripts/pre-commit.sh` |
| 22 | +**Pattern**: Shell script validation tool |
| 23 | + |
| 24 | +### Module Structure Requirements |
| 25 | + |
| 26 | +- [ ] Follow existing script structure and patterns in `scripts/pre-commit.sh` |
| 27 | +- [ ] Use consistent formatting and messaging style |
| 28 | +- [ ] Maintain the step-based execution model |
| 29 | + |
| 30 | +### Architectural Constraints |
| 31 | + |
| 32 | +- [ ] Must run as the last step in the `STEPS` array |
| 33 | +- [ ] Should use existing `cargo cov-check` alias from `.cargo/config.toml` |
| 34 | +- [ ] Must leverage existing step execution infrastructure (timing, error handling, formatting) |
| 35 | +- [ ] Must provide clear, actionable messaging |
| 36 | + |
| 37 | +### Anti-Patterns to Avoid |
| 38 | + |
| 39 | +- ❌ Adding special-case logic instead of using the existing step array |
| 40 | +- ❌ Running coverage before other checks (wastes time if linting fails) |
| 41 | +- ❌ Duplicating timing/formatting logic that already exists |
| 42 | + |
| 43 | +## Specifications |
| 44 | + |
| 45 | +### Current Pre-commit Structure |
| 46 | + |
| 47 | +The `scripts/pre-commit.sh` script uses a step-based execution model with an array of checks: |
| 48 | + |
| 49 | +```bash |
| 50 | +declare -a STEPS=( |
| 51 | + "Checking for unused dependencies (cargo machete)|No unused dependencies found|||cargo machete" |
| 52 | + "Running linters|All linters passed|||cargo run --bin linter all" |
| 53 | + "Running tests|All tests passed|||cargo test" |
| 54 | + "Testing cargo documentation|Documentation builds successfully|||cargo doc --no-deps --bins --examples --workspace --all-features" |
| 55 | + "Running comprehensive E2E tests|All E2E tests passed|(Filtering logs to WARNING level and above - this may take a few minutes)|RUST_LOG=warn|cargo run --bin e2e-tests-full" |
| 56 | +) |
| 57 | +``` |
| 58 | + |
| 59 | +Each step follows the format: |
| 60 | +`"description|success_message|optional_pre_message|optional_env_var|command"` |
| 61 | + |
| 62 | +All steps fail fast on errors due to `set -euo pipefail`. |
| 63 | + |
| 64 | +### Proposed Change |
| 65 | + |
| 66 | +Add coverage check as the **last step** in the `STEPS` array: |
| 67 | + |
| 68 | +```bash |
| 69 | +declare -a STEPS=( |
| 70 | + "Checking for unused dependencies (cargo machete)|No unused dependencies found|||cargo machete" |
| 71 | + "Running linters|All linters passed|||cargo run --bin linter all" |
| 72 | + "Running tests|All tests passed|||cargo test" |
| 73 | + "Testing cargo documentation|Documentation builds successfully|||cargo doc --no-deps --bins --examples --workspace --all-features" |
| 74 | + "Running comprehensive E2E tests|All E2E tests passed|(Filtering logs to WARNING level and above - this may take a few minutes)|RUST_LOG=warn|cargo run --bin e2e-tests-full" |
| 75 | + "Running code coverage check|Coverage meets 85% threshold|(Informational only - does not block commits)||cargo cov-check" |
| 76 | +) |
| 77 | +``` |
| 78 | + |
| 79 | +### Why This Approach? |
| 80 | + |
| 81 | +**Advantages:** |
| 82 | + |
| 83 | +- ✅ **Consistent**: Follows existing script patterns |
| 84 | +- ✅ **Simple**: Just one line added to the array |
| 85 | +- ✅ **Maintainable**: No special-case logic needed |
| 86 | +- ✅ **Clear**: Uses same messaging format as other steps |
| 87 | +- ✅ **Automatic timing**: Built-in timing display like other steps |
| 88 | + |
| 89 | +**Behavior:** |
| 90 | + |
| 91 | +- Runs last after all other checks pass |
| 92 | +- Still fails fast if coverage is below 85% (exits with non-zero code) |
| 93 | +- Shows clear error message from `cargo cov-check` |
| 94 | +- Uses existing error handling and timing infrastructure |
| 95 | + |
| 96 | +### Alternative: Non-blocking Implementation |
| 97 | + |
| 98 | +**Current decision**: Treat coverage like other mandatory checks (blocking). |
| 99 | + |
| 100 | +**If non-blocking behavior is needed later**, the script could be modified to: |
| 101 | + |
| 102 | +1. Add a separate section after the STEPS loop |
| 103 | +2. Use conditional execution (`if cargo cov-check; then ... else ... fi`) |
| 104 | +3. Exit with code 0 regardless of coverage result |
| 105 | + |
| 106 | +However, the simpler approach is to start with blocking behavior and only add complexity if feedback shows it's needed. |
| 107 | + |
| 108 | +## Implementation Plan |
| 109 | + |
| 110 | +### Phase 1: Add Coverage Step (5 minutes) |
| 111 | + |
| 112 | +- [ ] Open `scripts/pre-commit.sh` |
| 113 | +- [ ] Locate the `declare -a STEPS=()` array |
| 114 | +- [ ] Add new line at the end of the array (before the closing parenthesis) |
| 115 | +- [ ] The new line should be: |
| 116 | + |
| 117 | +```bash |
| 118 | +"Running code coverage check|Coverage meets 85% threshold|(Informational only - does not block commits)||cargo cov-check" |
| 119 | +``` |
| 120 | + |
| 121 | +- [ ] Save the file |
| 122 | + |
| 123 | +### Phase 2: Testing (10 minutes) |
| 124 | + |
| 125 | +- [ ] Run `./scripts/pre-commit.sh` on current codebase (coverage should pass at 85.75%) |
| 126 | +- [ ] Verify coverage check runs last (after E2E tests) |
| 127 | +- [ ] Verify timing is displayed correctly (uses existing timing infrastructure) |
| 128 | +- [ ] Verify success message appears when coverage passes |
| 129 | +- [ ] Verify script shows clear output from `cargo cov-check` |
| 130 | + |
| 131 | +## Acceptance Criteria |
| 132 | + |
| 133 | +**Quality Checks**: |
| 134 | + |
| 135 | +- [ ] Pre-commit checks pass: `./scripts/pre-commit.sh` |
| 136 | + |
| 137 | +**Task-Specific Criteria**: |
| 138 | + |
| 139 | +- [ ] Coverage check is added to `scripts/pre-commit.sh` |
| 140 | + **Task-Specific Criteria**: |
| 141 | + |
| 142 | +- [ ] Coverage check added as last step in `STEPS` array in `scripts/pre-commit.sh` |
| 143 | +- [ ] Uses `cargo cov-check` alias (already exists in `.cargo/config.toml`) |
| 144 | +- [ ] Success message reads: "Coverage meets 85% threshold" |
| 145 | +- [ ] Pre-message reads: "(Informational only - does not block commits)" |
| 146 | +- [ ] Script exits with non-zero code if coverage is below 85% |
| 147 | +- [ ] Timing information is automatically displayed (inherited from step infrastructure) |
| 148 | +- [ ] All existing checks still work correctly |
| 149 | + |
| 150 | +## Related Documentation |
| 151 | + |
| 152 | +- [Pre-commit Process](../contributing/commit-process.md) - Pre-commit workflow |
| 153 | +- [Development Principles](../development-principles.md) - Quality standards |
| 154 | +- [Testing Conventions](../contributing/testing.md) - Testing guidelines |
| 155 | + |
| 156 | +## Notes |
| 157 | + |
| 158 | +- The coverage check is intentionally non-blocking to support urgent fixes and patches |
| 159 | +- Running coverage last ensures developers don't waste time waiting for coverage if linting fails |
| 160 | +- The `tail -5` filter keeps output concise while showing the essential coverage summary |
| 161 | +- Exit code 0 is important for CI/CD pipelines that may use this script |
0 commit comments