Arkive/arkive

Fork 0

Consolidate the test harness and add coverage measurement #60

New issue

Closed

opened 2026-08-09 23:05:09 -04:00 by McJuniorstein · 3 comments

McJuniorstein commented

2026-08-09 23:05:09 -04:00

Member

Goal

Consolidate the test harness and add coverage measurement, so that the growing validator and processing code can be verified mechanically rather than by inspection.

Why

The suite has grown from 8 tests to 150 in about two weeks, and four things have accumulated that are worth fixing together rather than one at a time.

No coverage measurement — the substantive gap. During the review of PR #56 a validator cross-field rule was found to have become unreachable: a schema-level requirement had made it impossible for the rule to fire, so it read as enforcement while providing none. It was found by reasoning about the code, not by tooling. pipeline/validate/validate_records.py is now 838 lines carrying five distinct rule families, and "is every rule reachable and actually exercised?" is no longer a question anyone can answer by inspection. A rule that cannot fire is indistinguishable from a rule that works.

Two different ways of reaching the code under test. Three test modules load the validator through importlib.util.spec_from_file_location; five import the package normally:

spec_from_file_location:  test_validate_records.py, test_acquisition_schemas.py,
                          test_storage_boundary.py
normal package import:    test_package_import.py, test_file_integrity.py,
                          test_plain_text.py, test_processing.py, test_pdf_text.py

This is not inconsistency for its own sake. The validator lives at pipeline/validate/validate_records.py as a script rather than in src/arkive/, and docs/development.md states that tests "should not add repository paths to sys.path". The importlib bootstrap is a legal workaround for the same underlying problem, repeated three times.

A fixture helper borrowed across unrelated classes. tests/test_acquisition_schemas.py contains variant = AcquisitionContractTests.variant in four separate TestCase classes, binding one class's method onto others. It works, but a shared mixin or module-level helper is the honest shape and will not surprise the next contributor.

Hand-rolled global patching. tests/test_storage_boundary.py saves and restores validator.DATA_DIR around each test so that contamination fixtures are never written into the real data/ tree. That is the correct intent — a fixture inside data/ would be the very thing the guard prevents — but module-global mutation with manual restore is fragile, and would break the moment tests run in parallel.

Scope

Add a shared test support module providing the validator handle, the fixture copy-and-mutate helper, and a context manager for redirecting the core store path.
Move the reusable validator implementation into src/arkive/, keeping pipeline/validate/validate_records.py as a thin CLI wrapper so the canonical command still works. docs/development.md requires a scoped issue for this move; this issue is that scope.
Include the storage-boundary and symlink regression cases from Issue #45 in the cleaned-up test structure and the coverage audit.
Replace the four variant = AcquisitionContractTests.variant bindings with a shared helper.
Replace the manual DATA_DIR save/restore with a context manager or unittest.mock.patch.object.
Add statement and branch coverage measurement over src/ and pipeline/, with missing lines and branches visible during the audit, and a documented command in docs/development.md. coverage.py is the intended tool.
Record the current coverage figure as a baseline in the completion record, and audit every uncovered validator branch.
Update AGENTS.md and docs/development.md so the documented validation baseline matches what the project actually runs.

An uncovered branch is not automatically dead code

Coverage reports that a branch did not execute. It cannot show that a branch can never execute. Every uncovered validator branch therefore gets a recorded verdict rather than an assumption:

reachable but untested — add a focused regression test;
unreachable because an earlier schema or validation layer prevents it;
redundant, duplicating another rule;
superseded by the current contract.

Only the last three are removed, and the reason is recorded. The unreachable evidence rule found during Issue #44 is an example of the third category: a schema-level requirement made a cross-field rule impossible to trigger, so it read as enforcement while providing none.

Shared test support must not hide the problem

If the validator moves into the package, tests import it normally. The shared test-support layer must not wrap spec_from_file_location in a helper and call that progress — that would preserve the workaround behind a nicer name.

Shared support should contain genuinely test-specific utilities: fixture copy-and-mutate helpers, temporary core-root handling, and similar reusable infrastructure.

Out of scope

Replacing unittest with another test runner. The canonical command stays python -m unittest discover -s tests. This is explicitly not a migration to pytest or pytest-cov.
Adding CI, pre-commit hooks, or any automation that runs outside a contributor's machine.
Adding or replacing linting and formatting tools. Ruff stays as configured.
Enforcing a minimum coverage threshold. This issue measures and audits; whether a minimum is worth enforcing is a separate decision to make once real baseline data exists.
Embedding a coverage percentage in stable documentation. docs/development.md documents how to run coverage; a percentage there would be stale immediately. The baseline figure belongs in the completion record for this issue.
Rewriting existing test assertions or changing what any test checks.
Any change to schemas, policy documents, or committed records.

Dependencies

#45 — Define the bundled-core and user-local storage boundary

Sequencing agreed in review: #45 establishes the final storage-boundary behaviour, including the symlink cases found during its review, and this issue then refactors and strengthens the infrastructure against that completed baseline. This issue is to be taken before the remaining User-Acquired Content Overlay implementation issues.

Acceptance criteria

One documented way for a test to reach the validator, used consistently, and no spec_from_file_location where a normal package import is possible.
No test binds another class's method onto itself.
The core store path is redirected through a context manager rather than manual save and restore, and no test can leave it mutated on failure.
Statement and branch coverage can be measured with a single documented command, with missing lines and branches visible.
A baseline coverage figure is recorded in the completion record, not in stable documentation.
Every uncovered validator branch has a recorded verdict: reachable-but-untested, unreachable, redundant, or superseded.
The complete suite passes before and after, and the test count does not decrease. New focused regression tests may be added where the audit finds reachable behaviour that was previously untested.
AGENTS.md and docs/development.md describe the actual commands.
No CI, pre-commit, or automation outside a contributor's machine is introduced.

Validation

Run the complete unit-test suite before and after; the count must not decrease.
Run the metadata validator against the committed corpus.
Run ruff check and ruff format --check.
Run the new coverage command and record the result.
Confirm no schema, policy, or committed record changed.
Run git diff --check.

Suggested branch

feature/test-harness-consolidation

Notes

Coverage tooling adds a development dependency, which AGENTS.md otherwise restricts. That restriction is the reason this is a scoped issue rather than something folded into unrelated work. The argument for accepting the dependency is the unreachable rule already found in PR #56: without measurement, the next one is found the same way, by someone happening to reason about it during review.

Raised from work on Issues #44 and #45, where the duplication was introduced and noticed. Scope refined following review discussion on this issue: branch coverage rather than line coverage alone, the corrected test-count criterion, the uncovered-versus-unreachable distinction, normal package imports rather than a wrapped workaround, and a recorded baseline without an enforced threshold.

## Goal Consolidate the test harness and add coverage measurement, so that the growing validator and processing code can be verified mechanically rather than by inspection. ## Why The suite has grown from 8 tests to 150 in about two weeks, and four things have accumulated that are worth fixing together rather than one at a time. **No coverage measurement — the substantive gap.** During the review of PR #56 a validator cross-field rule was found to have become unreachable: a schema-level requirement had made it impossible for the rule to fire, so it read as enforcement while providing none. It was found by reasoning about the code, not by tooling. `pipeline/validate/validate_records.py` is now 838 lines carrying five distinct rule families, and "is every rule reachable and actually exercised?" is no longer a question anyone can answer by inspection. A rule that cannot fire is indistinguishable from a rule that works. **Two different ways of reaching the code under test.** Three test modules load the validator through `importlib.util.spec_from_file_location`; five import the package normally: ``` spec_from_file_location: test_validate_records.py, test_acquisition_schemas.py, test_storage_boundary.py normal package import: test_package_import.py, test_file_integrity.py, test_plain_text.py, test_processing.py, test_pdf_text.py ``` This is not inconsistency for its own sake. The validator lives at `pipeline/validate/validate_records.py` as a script rather than in `src/arkive/`, and `docs/development.md` states that tests "should not add repository paths to `sys.path`". The `importlib` bootstrap is a legal workaround for the same underlying problem, repeated three times. **A fixture helper borrowed across unrelated classes.** `tests/test_acquisition_schemas.py` contains `variant = AcquisitionContractTests.variant` in four separate `TestCase` classes, binding one class's method onto others. It works, but a shared mixin or module-level helper is the honest shape and will not surprise the next contributor. **Hand-rolled global patching.** `tests/test_storage_boundary.py` saves and restores `validator.DATA_DIR` around each test so that contamination fixtures are never written into the real `data/` tree. That is the correct intent — a fixture inside `data/` would be the very thing the guard prevents — but module-global mutation with manual restore is fragile, and would break the moment tests run in parallel. ## Scope - Add a shared test support module providing the validator handle, the fixture copy-and-mutate helper, and a context manager for redirecting the core store path. - Move the reusable validator implementation into `src/arkive/`, keeping `pipeline/validate/validate_records.py` as a thin CLI wrapper so the canonical command still works. `docs/development.md` requires a scoped issue for this move; this issue is that scope. - Include the storage-boundary and symlink regression cases from Issue #45 in the cleaned-up test structure and the coverage audit. - Replace the four `variant = AcquisitionContractTests.variant` bindings with a shared helper. - Replace the manual `DATA_DIR` save/restore with a context manager or `unittest.mock.patch.object`. - Add **statement and branch** coverage measurement over `src/` and `pipeline/`, with missing lines and branches visible during the audit, and a documented command in `docs/development.md`. `coverage.py` is the intended tool. - Record the current coverage figure as a baseline in the completion record, and audit every uncovered validator branch. - Update `AGENTS.md` and `docs/development.md` so the documented validation baseline matches what the project actually runs. ## An uncovered branch is not automatically dead code Coverage reports that a branch did not execute. It cannot show that a branch *can never* execute. Every uncovered validator branch therefore gets a recorded verdict rather than an assumption: - **reachable but untested** — add a focused regression test; - **unreachable** because an earlier schema or validation layer prevents it; - **redundant**, duplicating another rule; - **superseded** by the current contract. Only the last three are removed, and the reason is recorded. The unreachable evidence rule found during Issue #44 is an example of the third category: a schema-level requirement made a cross-field rule impossible to trigger, so it read as enforcement while providing none. ## Shared test support must not hide the problem If the validator moves into the package, tests import it normally. The shared test-support layer must not wrap `spec_from_file_location` in a helper and call that progress — that would preserve the workaround behind a nicer name. Shared support should contain genuinely test-specific utilities: fixture copy-and-mutate helpers, temporary core-root handling, and similar reusable infrastructure. ## Out of scope - Replacing `unittest` with another test runner. The canonical command stays `python -m unittest discover -s tests`. This is explicitly not a migration to pytest or pytest-cov. - Adding CI, pre-commit hooks, or any automation that runs outside a contributor's machine. - Adding or replacing linting and formatting tools. Ruff stays as configured. - Enforcing a minimum coverage threshold. This issue measures and audits; whether a minimum is worth enforcing is a separate decision to make once real baseline data exists. - Embedding a coverage percentage in stable documentation. `docs/development.md` documents **how to run** coverage; a percentage there would be stale immediately. The baseline figure belongs in the completion record for this issue. - Rewriting existing test assertions or changing what any test checks. - Any change to schemas, policy documents, or committed records. ## Dependencies - #45 — Define the bundled-core and user-local storage boundary Sequencing agreed in review: #45 establishes the final storage-boundary behaviour, including the symlink cases found during its review, and this issue then refactors and strengthens the infrastructure against that completed baseline. This issue is to be taken before the remaining User-Acquired Content Overlay implementation issues. ## Acceptance criteria - [ ] One documented way for a test to reach the validator, used consistently, and no `spec_from_file_location` where a normal package import is possible. - [ ] No test binds another class's method onto itself. - [ ] The core store path is redirected through a context manager rather than manual save and restore, and no test can leave it mutated on failure. - [ ] Statement **and branch** coverage can be measured with a single documented command, with missing lines and branches visible. - [ ] A baseline coverage figure is recorded in the completion record, not in stable documentation. - [ ] Every uncovered validator branch has a recorded verdict: reachable-but-untested, unreachable, redundant, or superseded. - [ ] The complete suite passes before and after, and the test count does not decrease. New focused regression tests may be added where the audit finds reachable behaviour that was previously untested. - [ ] `AGENTS.md` and `docs/development.md` describe the actual commands. - [ ] No CI, pre-commit, or automation outside a contributor's machine is introduced. ## Validation - Run the complete unit-test suite before and after; the count must not decrease. - Run the metadata validator against the committed corpus. - Run `ruff check` and `ruff format --check`. - Run the new coverage command and record the result. - Confirm no schema, policy, or committed record changed. - Run `git diff --check`. ## Suggested branch `feature/test-harness-consolidation` ## Notes Coverage tooling adds a development dependency, which `AGENTS.md` otherwise restricts. That restriction is the reason this is a scoped issue rather than something folded into unrelated work. The argument for accepting the dependency is the unreachable rule already found in PR #56: without measurement, the next one is found the same way, by someone happening to reason about it during review. Raised from work on Issues #44 and #45, where the duplication was introduced and noticed. Scope refined following review discussion on this issue: branch coverage rather than line coverage alone, the corrected test-count criterion, the uncovered-versus-unreachable distinction, normal package imports rather than a wrapped workaround, and a recorded baseline without an enforced threshold.

McJuniorstein added the

type/testing

type/implementation

labels

2026-08-09 23:05:09 -04:00

JackFrostbyte commented

2026-08-10 01:02:59 -04:00

Owner

I reviewed this issue in detail and I agree that it identifies real technical debt that is worth addressing now rather than allowing it to accumulate further.

My initial intention was actually to tackle #60 immediately, because the validator/test infrastructure has now grown enough that the problems identified here are becoming increasingly relevant:

validate_records.py has grown into substantial reusable application logic rather than remaining a small repository utility.
Tests still have to load it manually instead of importing the validator normally from the installed arkive package.
Some acquisition-schema tests reuse helper methods by borrowing them from another TestCase class rather than through explicit shared test-support code.
Some tests temporarily mutate validator globals such as DATA_DIR, which works but is increasingly fragile as more storage-boundary behavior gets added.
We currently have no systematic coverage measurement to tell us which validator rules and branches are actually being exercised.
Most importantly, previous review work has already exposed validator logic that had become unreachable after schema-level requirements changed. That was discovered manually rather than automatically.

However, while reviewing PR #59 for Issue #45, I found a blocking problem in the new storage-boundary enforcement: a symlink located inside the bundled core can resolve to a target outside the core before the validator decides whether the path is under data/, potentially allowing the contamination check to be bypassed.

I have therefore stopped before beginning #60 and left corrective review feedback on PR #59.

I want that behavior fixed in the PR where it was introduced, including an appropriate regression test, rather than allowing #59 to merge with a known boundary weakness and expecting #60 to clean it up afterward.

There are also two documentation-contract corrections requested in #59:

clarify the Windows local-storage permission requirement instead of describing POSIX mode 0700 as though it applied cross-platform;
correct a small distribution_scope wording inconsistency so the architecture document matches the actual schema contract.

Once those corrections are made, I will review PR #59 again. If everything is correct, I intend to merge it and close Issue #45.

After that, I want #60 to be the next issue tackled before continuing with the User-Acquired Content Overlay implementation issues.

I think this is the right sequencing because #59/#45 should first establish the final storage-boundary behavior, including the newly identified symlink case. Then #60 can refactor and strengthen the validator/test infrastructure against that completed baseline. The later implementation work can subsequently build on a cleaner and more inspectable foundation.

While reviewing #60, I also identified a few points I think should be incorporated when implementing it.

Coverage should include branch coverage

I do not think simple statement/line coverage is sufficient for this validator.

A large portion of the validator consists of conditional rules. Knowing that a line executed does not necessarily tell us whether all meaningful paths through the rule were tested.

When coverage tooling is introduced, I therefore want both statement and branch coverage, with missing lines/branches visible during the audit.

I am fine with adding coverage.py as a development dependency for this purpose. I do not want this issue to turn into a migration away from unittest or an introduction of pytest/pytest-cov. The existing test framework can remain.

The acceptance criterion regarding test count should be adjusted

The current wording says that the number of tests should remain the same before and after unless an unreachable rule is discovered.

That is slightly contradictory with another objective of this issue.

Coverage may discover a validator rule that is perfectly valid and reachable but simply has no regression test yet. In that situation, the correct response is to add a test, which naturally increases the test count even though no unreachable code was found.

I would therefore use the following principle instead:

The complete test suite must pass before and after the refactor, and the test count must not decrease. New focused regression tests may be added when the coverage audit identifies reachable behavior that was previously untested.

An uncovered branch is not automatically dead code

I also want to distinguish between "not covered by tests" and "unreachable."

Coverage can tell us that a branch was not executed. It cannot by itself prove that the branch can never execute.

For every uncovered validator branch, the implementation should determine why it is uncovered:

if it represents valid reachable behavior, add a focused regression test;
if it has genuinely become impossible because an earlier schema or validation layer prevents reaching it;
if it duplicates another rule;
or if it has been superseded by the current contract,

then remove it and record the reason.

The unreachable evidence rule found during the earlier acquisition work is a good example of the latter case.

I support moving reusable validator logic under `src/arkive/`

The validator has outgrown the role of a standalone repository script.

If this refactor is clean, my preferred end state is roughly:

src/arkive/
    <validator implementation>

pipeline/validate/validate_records.py
    <thin CLI/compatibility wrapper>

The existing canonical command can therefore continue to work, while tests and other Arkive code can import the actual implementation normally through the installed package.

If we make that move, I do not want the new shared test-support layer to simply hide the current spec_from_file_location workaround behind another helper. Tests should import the production package normally.

Shared test support should instead contain genuinely test-specific utilities such as fixture-copy/mutation helpers, temporary core-root handling, and similar reusable test infrastructure.

Global validator state should be handled safely

The existing pattern of manually replacing validator.DATA_DIR and later restoring it works, but this kind of global mutation becomes increasingly dangerous as the suite grows.

I would prefer that #60 provide an explicit safe mechanism such as a context manager or mock.patch.object()-based helper so restoration is automatic even if a test fails.

The new storage-boundary and symlink regression cases from #59 should be included in this cleaned-up structure.

Establish a coverage baseline, but do not create a threshold yet

I want #60 to establish Arkive's first meaningful coverage baseline.

However, I do not want us to arbitrarily decide that Arkive must immediately meet some percentage such as 80%, 90%, etc.

First measure what we actually have, audit the validator rules, add the missing tests that have meaningful value, and remove genuinely dead logic.

The resulting percentage can be recorded in the PR/Issue #60 completion record as the baseline for that commit.

Stable documentation such as docs/development.md should document how to run coverage, rather than embedding a percentage that will immediately become stale.

A future issue can decide whether enforcing a minimum coverage threshold in CI has enough value once we have real baseline data.

Intended result

At the end of #60, I would like us to have:

A normally importable validator implementation under the Arkive package, if the migration proves clean.
The existing validator CLI preserved as a thin entry point.
No manual spec_from_file_location loading where normal imports are possible.
Explicit shared test-support utilities instead of helpers borrowed between unrelated TestCase classes.
Safe temporary handling of validator paths/global state.
coverage.py included in the development tooling.
Statement and branch coverage available through a documented canonical command.
Every uncovered validator branch reviewed rather than blindly chasing a percentage.
Regression tests added for reachable behavior that currently has no test.
Unreachable/redundant/superseded validator logic removed where appropriate and the reason documented.
The post-#59 storage-boundary and symlink behavior included in that audit.
The first coverage baseline recorded without introducing an arbitrary enforcement threshold.
AGENTS.md / development documentation updated as necessary so future contributors use the same canonical validation and coverage workflow.

So I agree with raising #60 and I want it handled promptly.

I am only deliberately pausing it until the corrective work on PR #59 is finished, because the defect discovered there belongs to the behavior introduced by #59 and should be fixed and regression-tested there.

Once #59 is corrected, reviewed, and merged, please take #60 next before continuing with the later User-Acquired Content Overlay implementation issues.

In the meantime I will continue working on the next currently available lower dependency branch so this does not block unrelated progress.

I reviewed this issue in detail and I agree that it identifies real technical debt that is worth addressing now rather than allowing it to accumulate further. My initial intention was actually to tackle #60 immediately, because the validator/test infrastructure has now grown enough that the problems identified here are becoming increasingly relevant: * `validate_records.py` has grown into substantial reusable application logic rather than remaining a small repository utility. * Tests still have to load it manually instead of importing the validator normally from the installed `arkive` package. * Some acquisition-schema tests reuse helper methods by borrowing them from another `TestCase` class rather than through explicit shared test-support code. * Some tests temporarily mutate validator globals such as `DATA_DIR`, which works but is increasingly fragile as more storage-boundary behavior gets added. * We currently have no systematic coverage measurement to tell us which validator rules and branches are actually being exercised. * Most importantly, previous review work has already exposed validator logic that had become unreachable after schema-level requirements changed. That was discovered manually rather than automatically. However, while reviewing PR #59 for Issue #45, I found a blocking problem in the new storage-boundary enforcement: a symlink located inside the bundled core can resolve to a target outside the core before the validator decides whether the path is under `data/`, potentially allowing the contamination check to be bypassed. I have therefore stopped before beginning #60 and left corrective review feedback on PR #59. I want that behavior fixed in the PR where it was introduced, including an appropriate regression test, rather than allowing #59 to merge with a known boundary weakness and expecting #60 to clean it up afterward. There are also two documentation-contract corrections requested in #59: * clarify the Windows local-storage permission requirement instead of describing POSIX mode `0700` as though it applied cross-platform; * correct a small `distribution_scope` wording inconsistency so the architecture document matches the actual schema contract. Once those corrections are made, I will review PR #59 again. If everything is correct, I intend to merge it and close Issue #45. **After that, I want #60 to be the next issue tackled before continuing with the User-Acquired Content Overlay implementation issues.** I think this is the right sequencing because #59/#45 should first establish the final storage-boundary behavior, including the newly identified symlink case. Then #60 can refactor and strengthen the validator/test infrastructure against that completed baseline. The later implementation work can subsequently build on a cleaner and more inspectable foundation. While reviewing #60, I also identified a few points I think should be incorporated when implementing it. ### Coverage should include branch coverage I do not think simple statement/line coverage is sufficient for this validator. A large portion of the validator consists of conditional rules. Knowing that a line executed does not necessarily tell us whether all meaningful paths through the rule were tested. When coverage tooling is introduced, I therefore want both statement and **branch coverage**, with missing lines/branches visible during the audit. I am fine with adding `coverage.py` as a development dependency for this purpose. I do not want this issue to turn into a migration away from `unittest` or an introduction of pytest/pytest-cov. The existing test framework can remain. ### The acceptance criterion regarding test count should be adjusted The current wording says that the number of tests should remain the same before and after unless an unreachable rule is discovered. That is slightly contradictory with another objective of this issue. Coverage may discover a validator rule that is perfectly valid and reachable but simply has no regression test yet. In that situation, the correct response is to add a test, which naturally increases the test count even though no unreachable code was found. I would therefore use the following principle instead: > The complete test suite must pass before and after the refactor, and the test count must not decrease. New focused regression tests may be added when the coverage audit identifies reachable behavior that was previously untested. ### An uncovered branch is not automatically dead code I also want to distinguish between "not covered by tests" and "unreachable." Coverage can tell us that a branch was not executed. It cannot by itself prove that the branch can never execute. For every uncovered validator branch, the implementation should determine why it is uncovered: * if it represents valid reachable behavior, add a focused regression test; * if it has genuinely become impossible because an earlier schema or validation layer prevents reaching it; * if it duplicates another rule; * or if it has been superseded by the current contract, then remove it and record the reason. The unreachable evidence rule found during the earlier acquisition work is a good example of the latter case. ### I support moving reusable validator logic under `src/arkive/` The validator has outgrown the role of a standalone repository script. If this refactor is clean, my preferred end state is roughly: ```text src/arkive/ <validator implementation> pipeline/validate/validate_records.py <thin CLI/compatibility wrapper> ``` The existing canonical command can therefore continue to work, while tests and other Arkive code can import the actual implementation normally through the installed package. If we make that move, I do not want the new shared test-support layer to simply hide the current `spec_from_file_location` workaround behind another helper. Tests should import the production package normally. Shared test support should instead contain genuinely test-specific utilities such as fixture-copy/mutation helpers, temporary core-root handling, and similar reusable test infrastructure. ### Global validator state should be handled safely The existing pattern of manually replacing `validator.DATA_DIR` and later restoring it works, but this kind of global mutation becomes increasingly dangerous as the suite grows. I would prefer that #60 provide an explicit safe mechanism such as a context manager or `mock.patch.object()`-based helper so restoration is automatic even if a test fails. The new storage-boundary and symlink regression cases from #59 should be included in this cleaned-up structure. ### Establish a coverage baseline, but do not create a threshold yet I want #60 to establish Arkive's first meaningful coverage baseline. However, I do **not** want us to arbitrarily decide that Arkive must immediately meet some percentage such as 80%, 90%, etc. First measure what we actually have, audit the validator rules, add the missing tests that have meaningful value, and remove genuinely dead logic. The resulting percentage can be recorded in the PR/Issue #60 completion record as the baseline for that commit. Stable documentation such as `docs/development.md` should document **how to run coverage**, rather than embedding a percentage that will immediately become stale. A future issue can decide whether enforcing a minimum coverage threshold in CI has enough value once we have real baseline data. ### Intended result At the end of #60, I would like us to have: 1. A normally importable validator implementation under the Arkive package, if the migration proves clean. 2. The existing validator CLI preserved as a thin entry point. 3. No manual `spec_from_file_location` loading where normal imports are possible. 4. Explicit shared test-support utilities instead of helpers borrowed between unrelated `TestCase` classes. 5. Safe temporary handling of validator paths/global state. 6. `coverage.py` included in the development tooling. 7. Statement and branch coverage available through a documented canonical command. 8. Every uncovered validator branch reviewed rather than blindly chasing a percentage. 9. Regression tests added for reachable behavior that currently has no test. 10. Unreachable/redundant/superseded validator logic removed where appropriate and the reason documented. 11. The post-#59 storage-boundary and symlink behavior included in that audit. 12. The first coverage baseline recorded without introducing an arbitrary enforcement threshold. 13. `AGENTS.md` / development documentation updated as necessary so future contributors use the same canonical validation and coverage workflow. So I agree with raising #60 and I want it handled promptly. I am only deliberately pausing it until the corrective work on PR #59 is finished, because the defect discovered there belongs to the behavior introduced by #59 and should be fixed and regression-tested there. Once #59 is corrected, reviewed, and merged, please take #60 next before continuing with the later User-Acquired Content Overlay implementation issues. In the meantime I will continue working on the next currently available lower dependency branch so this does not block unrelated progress.

McJuniorstein referenced this issue

2026-08-10 01:38:29 -04:00

Define the bundled-core and user-local storage boundary #59

McJuniorstein commented

2026-08-10 01:49:36 -04:00

Author

Member

Agreed on all of it, and on the sequencing. The issue body is updated so the refinements live in the issue rather than only in this thread.

Your correction to the test-count criterion was right, and the original wording was self-contradictory. I had written that the count should stay the same "unless a rule is found to be unreachable", which quietly assumed the only reason to change the count was deletion. An audit that finds reachable-but-untested behaviour should add tests, and that is a success rather than a deviation. Replaced with your principle: the suite passes before and after, the count does not decrease, and focused regression tests may be added where the audit finds untested reachable behaviour.

Branch coverage rather than line coverage. Recorded in scope and in the acceptance criteria, with missing lines and branches visible during the audit. coverage.py as a development dependency, no pytest or pytest-cov migration, unittest stays canonical.

Uncovered is not unreachable. Added as its own section with four verdicts — reachable-but-untested, unreachable, redundant, superseded — of which only the last three justify removal, each with a recorded reason. The Issue #44 evidence rule is cited as an example of the superseded case, since a schema-level requirement made it impossible to trigger.

Shared test support must not hide the workaround. Also its own section now: if the validator moves into the package, tests import it normally, and wrapping spec_from_file_location in a helper does not count as progress. Shared support holds genuinely test-specific utilities — fixture copy-and-mutate, temporary core-root handling — and nothing else.

Baseline without a threshold. docs/development.md documents how to run coverage; the percentage goes in this issue's completion record, since a number in stable documentation is stale the moment it is written. Whether to enforce a minimum is left as a separate decision once real data exists.

Validator migration. Reworded from "evaluate moving" to the end state you described: implementation under src/arkive/, pipeline/validate/validate_records.py retained as a thin CLI wrapper so the canonical command keeps working. docs/development.md requires a scoped issue for that move, and this issue is now explicitly that scope rather than a conditional.

Dependency added. #60 now depends on #45, with your sequencing rationale recorded: #45 establishes the final boundary behaviour including the symlink cases, then #60 refactors against that completed baseline, then the remaining overlay implementation issues build on the cleaner foundation.

One thing worth adding to the audit list. Fixing the symlink escape on PR #59 surfaced a third defect: validate_tree() returned success as soon as no record files were discovered, before reporting problems already accumulated during discovery. A tree whose only content was a symlinked directory printed "No record files found" and exited zero. It is fixed in #59, and it is a useful data point for this issue — that branch had existed since Issue #3 in June, was covered by no test, and was found only because a regression test failed for a reason I did not expect. It is exactly the class of thing a branch-coverage audit is for.

Ready to take #60 as soon as #59 is merged.

Agreed on all of it, and on the sequencing. The issue body is updated so the refinements live in the issue rather than only in this thread. **Your correction to the test-count criterion was right, and the original wording was self-contradictory.** I had written that the count should stay the same "unless a rule is found to be unreachable", which quietly assumed the only reason to change the count was deletion. An audit that finds reachable-but-untested behaviour should *add* tests, and that is a success rather than a deviation. Replaced with your principle: the suite passes before and after, the count does not decrease, and focused regression tests may be added where the audit finds untested reachable behaviour. **Branch coverage rather than line coverage.** Recorded in scope and in the acceptance criteria, with missing lines and branches visible during the audit. `coverage.py` as a development dependency, no pytest or pytest-cov migration, `unittest` stays canonical. **Uncovered is not unreachable.** Added as its own section with four verdicts — reachable-but-untested, unreachable, redundant, superseded — of which only the last three justify removal, each with a recorded reason. The Issue #44 evidence rule is cited as an example of the superseded case, since a schema-level requirement made it impossible to trigger. **Shared test support must not hide the workaround.** Also its own section now: if the validator moves into the package, tests import it normally, and wrapping `spec_from_file_location` in a helper does not count as progress. Shared support holds genuinely test-specific utilities — fixture copy-and-mutate, temporary core-root handling — and nothing else. **Baseline without a threshold.** `docs/development.md` documents how to run coverage; the percentage goes in this issue's completion record, since a number in stable documentation is stale the moment it is written. Whether to enforce a minimum is left as a separate decision once real data exists. **Validator migration.** Reworded from "evaluate moving" to the end state you described: implementation under `src/arkive/`, `pipeline/validate/validate_records.py` retained as a thin CLI wrapper so the canonical command keeps working. `docs/development.md` requires a scoped issue for that move, and this issue is now explicitly that scope rather than a conditional. **Dependency added.** #60 now depends on #45, with your sequencing rationale recorded: #45 establishes the final boundary behaviour including the symlink cases, then #60 refactors against that completed baseline, then the remaining overlay implementation issues build on the cleaner foundation. One thing worth adding to the audit list. Fixing the symlink escape on PR #59 surfaced a third defect: `validate_tree()` returned success as soon as no record files were discovered, *before* reporting problems already accumulated during discovery. A tree whose only content was a symlinked directory printed "No record files found" and exited zero. It is fixed in #59, and it is a useful data point for this issue — that branch had existed since Issue #3 in June, was covered by no test, and was found only because a regression test failed for a reason I did not expect. It is exactly the class of thing a branch-coverage audit is for. Ready to take #60 as soon as #59 is merged.

McJuniorstein added a new dependency

2026-08-10 01:49:57 -04:00

#45 Define the bundled-core and user-local storage/index boundary

McJuniorstein self-assigned this

2026-08-11 20:09:22 -04:00

McJuniorstein referenced this issue from a pull request that will close it,

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

Consolidate the test harness and add coverage measurement #62

JackFrostbyte referenced this issue

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

Consolidate the test harness and add coverage measurement #62

JackFrostbyte closed this issue

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

JackFrostbyte referenced this issue from a commit

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

Merge pull request 'Consolidate the test harness and add coverage measurement' (#62) from feature/issue-60-test-harness-consolidation into develop

JackFrostbyte commented

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

Owner

Issue #60 is complete via PR #62.

The test harness has been consolidated around normal package imports, the metadata validator now lives in src/arkive/metadata_validation.py, and the existing pipeline/validate/validate_records.py command is retained as a thin compatibility wrapper.

Shared test support has been centralized, fragile manual global-state restoration has been replaced with managed patching, and the validator coverage audit added focused regression tests for reachable behavior that previously lacked coverage.

Coverage now correctly measures both the packaged Arkive code and the pipeline compatibility wrapper, including execution through the real subprocess-based CLI tests. The validator and CLI wrapper both reach 100% coverage, with repository-wide coverage at 95%.

The final branch was also reconciled with current develop and validated with 237 passing tests, Ruff, formatting checks, the canonical validator, coverage, pip check, and git diff --check.

Closing as completed.

Issue #60 is complete via PR #62. The test harness has been consolidated around normal package imports, the metadata validator now lives in `src/arkive/metadata_validation.py`, and the existing `pipeline/validate/validate_records.py` command is retained as a thin compatibility wrapper. Shared test support has been centralized, fragile manual global-state restoration has been replaced with managed patching, and the validator coverage audit added focused regression tests for reachable behavior that previously lacked coverage. Coverage now correctly measures both the packaged Arkive code and the `pipeline` compatibility wrapper, including execution through the real subprocess-based CLI tests. The validator and CLI wrapper both reach 100% coverage, with repository-wide coverage at 95%. The final branch was also reconciled with current `develop` and validated with 237 passing tests, Ruff, formatting checks, the canonical validator, coverage, `pip check`, and `git diff --check`. Closing as completed.