Arkive/arkive

Fork 0

Consolidate the test harness and add coverage measurement #62

Merged

JackFrostbyte merged 4 commits from feature/issue-60-test-harness-consolidation into develop

2026-08-16 01:24:31 -04:00

McJuniorstein commented

2026-08-11 20:20:41 -04:00

Member

Summary

Moves the validator implementation to src/arkive/metadata_validation.py, keeping pipeline/validate/validate_records.py as a thin CLI wrapper so the documented command still works.
Adds tests/support.py with the shared helpers, removing the spec_from_file_location bootstrap from three test modules, the four borrowed variant bindings, and the hand-rolled DATA_DIR save-and-restore.
Adds coverage.py with branch coverage enabled, documented as a command in docs/development.md with no percentage embedded there.
Audits every uncovered validator branch and adds regression tests for the reachable behaviour found. Test count rises from 178 to 208.

The audit

The validator began at 89% statement and branch coverage: 38 uncovered statements and 30 partial branches.

Every one proved to be reachable behaviour with no regression test. No dead code was found. The one genuinely unreachable rule this project has had was the evidence check removed under Issue #44, and it was already gone.

Verdicts by group, using the four categories agreed on this issue:

Uncovered behaviour	Verdict	Action
JSON parse failure for all six record types	reachable, untested	tests added
Schema rejection for source, document, chunk	reachable, untested	tests added
Duplicate `source_id`, `document_id`, `rights_id`	reachable, untested	tests added
Document referencing unknown source; `derived_from` referencing unknown document; document derived from itself	reachable, untested	tests added
Chunk referencing unknown document or unknown source	reachable, untested	tests added
Reversed character offsets	reachable, untested	test added
Rejected source without `rejection_reason`	reachable, untested	test added
Rights profile referencing unknown source; recipe source not matching its rights profile	reachable, untested	tests added
Recipe that does not require acknowledgement	reachable, untested	test added
Unrecognized record filename	reachable, untested	test added
CLI help, missing path, empty tree	reachable, untested	tests added
`_load_json` returning `None` for a vanished file	reachable only under a race between discovery and load	direct test added

Result: src/arkive/metadata_validation.py is now at 100% statement and branch coverage.

Repository total is 95%. The remainder is file_integrity.py at 85% and pdf_text.py at 90%, recorded here as the baseline rather than audited — this issue's criterion covers validator branches, and those modules deserve their own pass rather than a drive-by.

Baseline figures

Recorded here rather than in docs/development.md, since a percentage in stable documentation is stale the moment it is written.

Name                                Stmts   Miss Branch BrPart  Cover
src/arkive/__init__.py                  0      0      0      0   100%
src/arkive/file_integrity.py          261     32     94     18    85%
src/arkive/metadata_validation.py     370      0    230      0   100%
src/arkive/pdf_text.py                 59      7      8      0    90%
src/arkive/plain_text.py               28      0      6      0   100%
src/arkive/processing.py               59      0      2      0   100%
src/arkive/text_normalization.py      105      0     16      0   100%
TOTAL                                 882     39    356     18    95%

Acceptance criteria

One documented way for a test to reach the validator, and no spec_from_file_location where a normal import is possible — every test module now imports arkive.metadata_validation, directly or through tests/support.py. No file-path loading remains anywhere in tests/.
No test binds another class's method onto itself — the four variant = AcquisitionContractTests.variant bindings are replaced by ValidatorTestCase.fixture_variant.
The core store path is redirected through a context manager, and no test can leave it mutated on failure — support.core_store() and ValidatorTestCase.use_core_store() both use mock.patch.object, which restores on exception.
Statement and branch coverage from a single documented command, with missing lines and branches visible — branch = true and show_missing = true in pyproject.toml; command documented in docs/development.md.
A baseline figure recorded in the completion record, not in stable documentation — above. docs/development.md documents how to run coverage and deliberately carries no percentage.
Every uncovered validator branch has a recorded verdict — the table above; all reachable-but-untested, all now covered.
The suite passes before and after, and the count does not decrease — 178 before, 208 after. The 30 new tests are the regression coverage the audit identified, exactly the case your revised criterion allows for.
AGENTS.md and docs/development.md describe the actual commands — both updated, including the validator's new location and the wrapper's role.
No CI, pre-commit, or automation outside a contributor's machine — none added. unittest remains canonical; no pytest, no pytest-cov.

Verification

git diff --check
python pipeline/validate/validate_records.py — PASS, committed corpus unchanged
python -m unittest discover -s tests -v — 208 tests passing
python -m ruff check src pipeline tests
python -m ruff format --check src pipeline tests
python -m pip check
python -m coverage run -m unittest discover -s tests then report
Complete diff inspected

Points for reviewer attention

The documented command is tested as a subprocess. The real risk in this refactor is that pipeline/validate/validate_records.py quietly stops being runnable while every import-based test still passes. DocumentedCommandTests therefore invokes it with subprocess.run exactly as the docs instruct, covering the corpus run, --help, and the missing-path exit code.

One limitation introduced, and deliberately not solved. REPO_ROOT is still derived from __file__, which is correct for a source checkout and for the documented editable install but would not be for a non-editable install, where meta/schemas would need shipping as package data. There is no such distribution yet and [project.scripts] is still empty, so solving it now would be speculative. It is recorded as a comment at the constant.

The _load_json missing-file branch is tested directly, calling the private helper. It is reachable only if a record disappears between discovery and loading, which cannot be produced reliably through the public interface. Calling the helper seemed better than leaving a defensive path unexercised or deleting a guard that protects against a real race.

file_integrity.py and pdf_text.py were not audited. Their figures are recorded as the baseline. Both are worth a pass, but folding them in here would have widened the change well past this issue.

Linked issue

Closes #60

## Summary - Moves the validator implementation to `src/arkive/metadata_validation.py`, keeping `pipeline/validate/validate_records.py` as a thin CLI wrapper so the documented command still works. - Adds `tests/support.py` with the shared helpers, removing the `spec_from_file_location` bootstrap from three test modules, the four borrowed `variant` bindings, and the hand-rolled `DATA_DIR` save-and-restore. - Adds `coverage.py` with branch coverage enabled, documented as a command in `docs/development.md` with no percentage embedded there. - Audits every uncovered validator branch and adds regression tests for the reachable behaviour found. Test count rises from 178 to 208. ## The audit The validator began at **89%** statement and branch coverage: 38 uncovered statements and 30 partial branches. **Every one proved to be reachable behaviour with no regression test. No dead code was found.** The one genuinely unreachable rule this project has had was the evidence check removed under Issue #44, and it was already gone. Verdicts by group, using the four categories agreed on this issue: | Uncovered behaviour | Verdict | Action | |---|---|---| | JSON parse failure for all six record types | reachable, untested | tests added | | Schema rejection for source, document, chunk | reachable, untested | tests added | | Duplicate `source_id`, `document_id`, `rights_id` | reachable, untested | tests added | | Document referencing unknown source; `derived_from` referencing unknown document; document derived from itself | reachable, untested | tests added | | Chunk referencing unknown document or unknown source | reachable, untested | tests added | | Reversed character offsets | reachable, untested | test added | | Rejected source without `rejection_reason` | reachable, untested | test added | | Rights profile referencing unknown source; recipe source not matching its rights profile | reachable, untested | tests added | | Recipe that does not require acknowledgement | reachable, untested | test added | | Unrecognized record filename | reachable, untested | test added | | CLI help, missing path, empty tree | reachable, untested | tests added | | `_load_json` returning `None` for a vanished file | reachable only under a race between discovery and load | direct test added | **Result: `src/arkive/metadata_validation.py` is now at 100% statement and branch coverage.** Repository total is **95%**. The remainder is `file_integrity.py` at 85% and `pdf_text.py` at 90%, recorded here as the baseline rather than audited — this issue's criterion covers validator branches, and those modules deserve their own pass rather than a drive-by. ## Baseline figures Recorded here rather than in `docs/development.md`, since a percentage in stable documentation is stale the moment it is written. ``` Name Stmts Miss Branch BrPart Cover src/arkive/__init__.py 0 0 0 0 100% src/arkive/file_integrity.py 261 32 94 18 85% src/arkive/metadata_validation.py 370 0 230 0 100% src/arkive/pdf_text.py 59 7 8 0 90% src/arkive/plain_text.py 28 0 6 0 100% src/arkive/processing.py 59 0 2 0 100% src/arkive/text_normalization.py 105 0 16 0 100% TOTAL 882 39 356 18 95% ``` ## Acceptance criteria - [x] **One documented way for a test to reach the validator, and no `spec_from_file_location` where a normal import is possible** — every test module now imports `arkive.metadata_validation`, directly or through `tests/support.py`. No file-path loading remains anywhere in `tests/`. - [x] **No test binds another class's method onto itself** — the four `variant = AcquisitionContractTests.variant` bindings are replaced by `ValidatorTestCase.fixture_variant`. - [x] **The core store path is redirected through a context manager, and no test can leave it mutated on failure** — `support.core_store()` and `ValidatorTestCase.use_core_store()` both use `mock.patch.object`, which restores on exception. - [x] **Statement and branch coverage from a single documented command, with missing lines and branches visible** — `branch = true` and `show_missing = true` in `pyproject.toml`; command documented in `docs/development.md`. - [x] **A baseline figure recorded in the completion record, not in stable documentation** — above. `docs/development.md` documents how to run coverage and deliberately carries no percentage. - [x] **Every uncovered validator branch has a recorded verdict** — the table above; all reachable-but-untested, all now covered. - [x] **The suite passes before and after, and the count does not decrease** — 178 before, 208 after. The 30 new tests are the regression coverage the audit identified, exactly the case your revised criterion allows for. - [x] **`AGENTS.md` and `docs/development.md` describe the actual commands** — both updated, including the validator's new location and the wrapper's role. - [x] **No CI, pre-commit, or automation outside a contributor's machine** — none added. `unittest` remains canonical; no pytest, no pytest-cov. ## Verification - [x] `git diff --check` - [x] `python pipeline/validate/validate_records.py` — PASS, committed corpus unchanged - [x] `python -m unittest discover -s tests -v` — 208 tests passing - [x] `python -m ruff check src pipeline tests` - [x] `python -m ruff format --check src pipeline tests` - [x] `python -m pip check` - [x] `python -m coverage run -m unittest discover -s tests` then `report` - [x] Complete diff inspected ## Points for reviewer attention **The documented command is tested as a subprocess.** The real risk in this refactor is that `pipeline/validate/validate_records.py` quietly stops being runnable while every import-based test still passes. `DocumentedCommandTests` therefore invokes it with `subprocess.run` exactly as the docs instruct, covering the corpus run, `--help`, and the missing-path exit code. **One limitation introduced, and deliberately not solved.** `REPO_ROOT` is still derived from `__file__`, which is correct for a source checkout and for the documented editable install but would not be for a non-editable install, where `meta/schemas` would need shipping as package data. There is no such distribution yet and `[project.scripts]` is still empty, so solving it now would be speculative. It is recorded as a comment at the constant. **The `_load_json` missing-file branch is tested directly**, calling the private helper. It is reachable only if a record disappears between discovery and loading, which cannot be produced reliably through the public interface. Calling the helper seemed better than leaving a defensive path unexercised or deleting a guard that protects against a real race. **`file_integrity.py` and `pdf_text.py` were not audited.** Their figures are recorded as the baseline. Both are worth a pass, but folding them in here would have widened the change well past this issue. ## Linked issue Closes #60

McJuniorstein added 1 commit

2026-08-11 20:20:41 -04:00

Consolidate the test harness and add coverage measurement 33926785fc

Move the validator into the package, remove the duplicated test plumbing
it forced, and measure what the suite actually exercises.

Validator location:
- the implementation moves to src/arkive/metadata_validation.py, so tests
  and other Arkive code import it normally
- pipeline/validate/validate_records.py remains as a thin CLI wrapper, so
  the documented command keeps working, with a clear error if the package
  is not installed
- three subprocess tests run that wrapper the way the documentation says
  to, since the risk this refactor introduces is that the documented
  command silently stops working

Test support:
- tests/support.py holds the shared helpers: fixture copy-and-mutate,
  temporary directories, a validator runner, a CLI runner, and a core
  store redirect
- no test loads the validator through spec_from_file_location any more
- no test binds another TestCase's method onto itself; the four borrowed
  `variant` bindings are gone
- the core store redirect uses mock.patch.object, which restores state
  even when a test fails, rather than hand-rolled save and restore

Coverage:
- coverage.py added as a development dependency, with branch coverage
  enabled and missing lines and branches shown
- documented in docs/development.md as a command, with no percentage
  embedded there, and noted in AGENTS.md as available but not part of the
  required baseline

Audit result. The validator began at 89% statement and branch coverage,
with 38 uncovered statements and 30 partial branches. Every one proved to
be reachable behaviour with no regression test; no dead code was found,
the previously unreachable rule having already been removed under Issue
#44. Tests were added for JSON and schema rejection across all six record
types, duplicate source, document and rights identifiers, unknown source,
document and parent references, self-derived documents, reversed
character offsets, rejected sources without a reason, recipes not
requiring acknowledgement, unrecognized filenames, and the command line's
help, missing-path and empty-tree paths.

The validator now sits at 100% statement and branch coverage. Repository
total is 95%, the remainder being file_integrity.py at 85% and
pdf_text.py at 90%, which are recorded as the baseline rather than
audited here.

Test count rises from 178 to 208. No existing assertion was changed, and
no schema, policy, or committed record was touched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JackFrostbyte commented

2026-08-12 02:35:37 -04:00

Owner

The main blocker is the current coverage configuration.

Issue #60 explicitly calls for coverage measurement across src/, pipeline/, and the validator. The PR currently configures coverage with:

[tool.coverage.run]
branch = true
source = ["src/arkive", "pipeline"]

At first glance that looks correct, but pipeline/validate/ is not currently a regular Python package: it contains validate_records.py without an __init__.py.

Because of that, coverage.py does not automatically discover that file as an unexecuted source file when building the report. The coverage output shown in this PR confirms the problem: the report lists the modules under src/arkive, but there is no entry for:

pipeline/validate/validate_records.py

So although pipeline is present in the coverage configuration, the compatibility CLI wrapper is currently disappearing from the coverage report entirely.

There is also another related detail. The new CLI tests correctly execute the wrapper through subprocess.run(). I agree with that design because it tests the actual documented command instead of merely importing main().

However, a normal coverage run does not automatically collect execution from spawned Python subprocesses. So at the moment we have:

the wrapper is functionally tested through a real subprocess;
the subprocess execution is not contributing to coverage;
the wrapper itself is not even appearing as an uncovered file in the coverage report.

That means the acceptance goal of actually measuring the pipeline/ code is not yet satisfied.

Please adjust the coverage configuration so pipeline/validate/validate_records.py is visibly included in the report. Coverage supports namespace-package source discovery through include_namespace_packages = true, which should at minimum prevent this file from silently disappearing.

Ideally, please also configure subprocess coverage so that the CLI subprocess tests actually contribute coverage for the wrapper. Current coverage.py supports subprocess collection through its subprocess patching/configuration, combined with parallel data files and coverage combine.

The goal is that after the fix, the coverage report should visibly contain something similar to:

pipeline/validate/validate_records.py

and its executed lines/branches should reflect the CLI tests.

Please do not simply leave the existing source = ["src/arkive", "pipeline"] configuration and consider pipeline covered—the current report demonstrates that it is not actually being measured.

There are also two smaller cleanup items I noticed.

First, src/arkive/metadata_validation.py was added with executable mode 100755. Now that it is a normal library module and does not have a shebang, it should normally be 100644. Please remove the executable bit.

Second, the CHANGELOG currently describes the validator audit as finding:

thirty-eight uncovered reachable behaviours

The numbers in the PR appear to actually represent 38 uncovered statements plus 30 partial branches. Those are coverage metrics, not necessarily 38 separate behaviours. Please reword that entry so it accurately describes what was found rather than equating uncovered statements with independent behaviours.

Aside from these points, the implementation looks good from my review:

moving the validator into src/arkive/metadata_validation.py is the right architectural direction;
replacing the repeated dynamic spec_from_file_location loading with normal package imports is a clear improvement;
the shared test helpers are cleaner;
keeping pipeline/validate/validate_records.py as a thin compatibility CLI wrapper preserves the existing documented workflow;
testing that wrapper through an actual subprocess is the right way to protect the CLI contract;
the Issue #45 storage-boundary/symlink behavior appears to have been preserved;
the validator audit and added regression tests substantially improve confidence in the existing rules;
I did not see unrelated schema/policy/corpus changes mixed into this refactor.

So I am not asking for a redesign. This looks like a small final rework before merge.

Requested changes

Make coverage actually discover and report pipeline/validate/validate_records.py.
Preferably collect coverage from the subprocess CLI tests as well.
Remove the executable bit from src/arkive/metadata_validation.py.
Correct the CHANGELOG wording around the 38 uncovered statements / 30 partial branches.
Re-run the complete test suite, Ruff checks, validator, and coverage report after the corrections, and include the resulting coverage output showing the pipeline wrapper.

Once those points are addressed, I expect this PR to be ready for merge without another substantial round of changes.

Thanks for the work on this PR. I reviewed the refactor and the test/coverage changes in detail. The overall direction looks good, and I think this is very close to being ready to merge, but there are a few things I would like corrected first. The main blocker is the current coverage configuration. Issue #60 explicitly calls for coverage measurement across `src/`, `pipeline/`, and the validator. The PR currently configures coverage with: ```toml id="r0p4t1" [tool.coverage.run] branch = true source = ["src/arkive", "pipeline"] ``` At first glance that looks correct, but `pipeline/validate/` is not currently a regular Python package: it contains `validate_records.py` without an `__init__.py`. Because of that, coverage.py does not automatically discover that file as an unexecuted source file when building the report. The coverage output shown in this PR confirms the problem: the report lists the modules under `src/arkive`, but there is no entry for: ```text id="8ym2fk" pipeline/validate/validate_records.py ``` So although `pipeline` is present in the coverage configuration, the compatibility CLI wrapper is currently disappearing from the coverage report entirely. There is also another related detail. The new CLI tests correctly execute the wrapper through `subprocess.run()`. I agree with that design because it tests the actual documented command instead of merely importing `main()`. However, a normal coverage run does not automatically collect execution from spawned Python subprocesses. So at the moment we have: * the wrapper is functionally tested through a real subprocess; * the subprocess execution is not contributing to coverage; * the wrapper itself is not even appearing as an uncovered file in the coverage report. That means the acceptance goal of actually measuring the `pipeline/` code is not yet satisfied. Please adjust the coverage configuration so `pipeline/validate/validate_records.py` is visibly included in the report. Coverage supports namespace-package source discovery through `include_namespace_packages = true`, which should at minimum prevent this file from silently disappearing. Ideally, please also configure subprocess coverage so that the CLI subprocess tests actually contribute coverage for the wrapper. Current coverage.py supports subprocess collection through its subprocess patching/configuration, combined with parallel data files and `coverage combine`. The goal is that after the fix, the coverage report should visibly contain something similar to: ```text id="ke5n07" pipeline/validate/validate_records.py ``` and its executed lines/branches should reflect the CLI tests. Please do not simply leave the existing `source = ["src/arkive", "pipeline"]` configuration and consider `pipeline` covered—the current report demonstrates that it is not actually being measured. There are also two smaller cleanup items I noticed. First, `src/arkive/metadata_validation.py` was added with executable mode `100755`. Now that it is a normal library module and does not have a shebang, it should normally be `100644`. Please remove the executable bit. Second, the CHANGELOG currently describes the validator audit as finding: > thirty-eight uncovered reachable behaviours The numbers in the PR appear to actually represent **38 uncovered statements plus 30 partial branches**. Those are coverage metrics, not necessarily 38 separate behaviours. Please reword that entry so it accurately describes what was found rather than equating uncovered statements with independent behaviours. Aside from these points, the implementation looks good from my review: * moving the validator into `src/arkive/metadata_validation.py` is the right architectural direction; * replacing the repeated dynamic `spec_from_file_location` loading with normal package imports is a clear improvement; * the shared test helpers are cleaner; * keeping `pipeline/validate/validate_records.py` as a thin compatibility CLI wrapper preserves the existing documented workflow; * testing that wrapper through an actual subprocess is the right way to protect the CLI contract; * the Issue #45 storage-boundary/symlink behavior appears to have been preserved; * the validator audit and added regression tests substantially improve confidence in the existing rules; * I did not see unrelated schema/policy/corpus changes mixed into this refactor. So I am **not asking for a redesign**. This looks like a small final rework before merge. ### Requested changes 1. Make coverage actually discover and report `pipeline/validate/validate_records.py`. 2. Preferably collect coverage from the subprocess CLI tests as well. 3. Remove the executable bit from `src/arkive/metadata_validation.py`. 4. Correct the CHANGELOG wording around the `38 uncovered statements / 30 partial branches`. 5. Re-run the complete test suite, Ruff checks, validator, and coverage report after the corrections, and include the resulting coverage output showing the `pipeline` wrapper. Once those points are addressed, I expect this PR to be ready for merge without another substantial round of changes.

JackFrostbyte added 1 commit

2026-08-16 01:18:09 -04:00