Review testing routines

[maxnutz]

Review testing routines

Update 2026-07-08: This issue was originally opened against an older snapshot of
class_definitions.py (108 statements, ~66% coverage). The codebase has moved on
significantly since then. This body has been rewritten to reflect the current state,
answer the open structural questions, and give a concrete implementation plan. The
original coverage table is preserved in the issue's edit history.

Current coverage (as of main @ 4fc636a, measured with pixi run pytest tests/ --cov=pypsa_validation_processing --cov-report=term-missing)

module	statements	coverage
class_definitions.py	313	87%
statistics_functions.py	172	98%
utils.py	31	100%
workflow.py	24	96%
total	542	92%

The repo is already well past 80% overall. class_definitions.py is the weakest module and
the remaining gaps are narrow — mostly untested error branches (missing-path
FileNotFoundError/ValueError checks in __init__) and one multi-unit-parsing branch,
not a systemic hole. The README's testing-batch command referenced in the original body
no longer exists in README.md — that item appears already resolved.

Critical finding: silent test loss in test_statistics_functions.py

class TestFinalEnergyByCarrierOil is defined twice in
tests/test_statistics_functions.py:

• Line 368 — 13 test methods, custom _OilStatisticsAccessor mock
• Line 1150 — 6 test methods, its own separate _OilStatisticsAccessor mock

Python silently overwrites the first class binding with the second at import time, so
13 test methods never execute. pytest reports green with no warning — this is
exactly the class of risk a "review testing routines" issue should surface, independent
of the coverage percentage. No linter in this repo currently catches redefinition
(no ruff/flake8 config with F811 enabled, no pre-commit hook).

Action: merge the two blocks, keep whichever assertions are non-redundant, and add a
guard (see Phase 0 below) so this can't happen silently again.

Answers to the structural questions

Is the test structure efficient?
Partially. tests/test_statistics_functions.py is one 89 KB file with one TestX class
per statistics function — a reasonable pattern in principle, matching the "each
statistics function is independent" architecture in CLAUDE.md. But nothing enforces
uniqueness of class names within the file, which is how the Oil duplication above went
unnoticed. The file's size also makes review harder than it needs to be.

Would a common mock network across all statistics-function tests be better?
One already exists — tests/conftest.py provides MockPyPSANetwork,
MockStatisticsAccessor, and a mock_network fixture, and most tests use it. The
problem is inconsistent adoption: several test classes (e.g. both
TestFinalEnergyByCarrierOil blocks) hand-roll their own accessor subclass
(_OilStatisticsAccessor) instead of parametrizing the shared one. This is duplicated
setup logic and a contributor to file size. Consolidating onto the shared, parametrized
fixture is the right direction — see Phase 1.

Implementation plan

Target: push class_definitions.py and statistics_functions.py to 95%+, eliminate
duplicate/dead test code, and make mock usage consistent. Written to be picked up
directly by an implementing (AI) agent — each phase is independently shippable and
touches only tests/ (Phase 0–1) or adds tests for existing, unchanged production code
(Phase 2–3).

Phase 0 — Fix the duplicate-class bug (blocking, do first)

1. In tests/test_statistics_functions.py, rename or merge the two
   TestFinalEnergyByCarrierOil blocks (lines 368 and 1150). Prefer merging into one
   class; de-duplicate assertions that test the same behavior twice, keep the union of
   distinct scenarios covered by both _OilStatisticsAccessor variants.
2. Run pixi run pytest tests/test_statistics_functions.py -v and confirm the test count
   increases (currently 6 Oil tests run; merged, expect close to 13-19 depending on
   dedup) and all pass.
3. Add a cheap regression guard: a small test (e.g. in tests/test_utils.py or a new
   tests/test_no_duplicate_test_classes.py) that walks tests/*.py with ast and
   fails if any module defines the same top-level class name twice. This is cheaper than
   introducing a linter and is self-contained to the test suite.
   (Optional, separately proposable: enable ruff with F811 in CI to catch this
   category of bug in production code too, not just tests.)

Phase 1 — Consolidate mocks onto shared fixtures

1. Audit tests/test_statistics_functions.py for bespoke accessor subclasses (start
   with _OilStatisticsAccessor in both blocks; check other TestX classes for similar
   patterns).
2. Extend MockStatisticsAccessor in tests/conftest.py to accept the parameters those
   bespoke accessors need (e.g. rescom_empty, all_oil_value, all_oil_empty), either
   via constructor kwargs or a small factory fixture
   (mock_network_with(**overrides)) so individual test classes stop subclassing.
3. Migrate TestFinalEnergyByCarrierOil (merged from Phase 0) to use the shared,
   parametrized fixture instead of _OilStatisticsAccessor. Use this as the template.
4. Repeat for any other test classes found in step 1 that define their own accessor.
5. Delete the now-dead bespoke mock classes.
6. Do not touch pypsa_validation_processing/statistics_functions.py itself —
   this phase is test-only, per the "statistics functions are independent, tests don't
   change production behavior" rule.

Phase 2 — Close remaining class_definitions.py gaps (87% → 95%+)

Add targeted unit tests in tests/test_network_processor.py for the currently-missing
lines (re-check exact numbers with --cov-report=term-missing before starting, since
Phase 0/1 changes don't touch this file but line numbers may drift with unrelated
commits):

• __init__: FileNotFoundError when network_results_path doesn't exist;
  ValueError when definitions_path is unset; FileNotFoundError when
  definitions_path doesn't exist; ValueError when model_name/scenario_name is
  None.
• format_timestamps: the TypeError fallback branch in the pd.to_datetime call.
• The "function not found in statistics_functions.py" warning branch when resolving a
  variable to its implementation.
• The multi-unit-parsing branch ("[" in target_unit and "]" in target_unit) including
  both the successful ast.literal_eval path and the exception fallback to "TJ".
• Remaining KeyError branches around unit/variable lookup in common definitions.

Phase 3 — Close small remaining gaps in statistics_functions.py / workflow.py

• statistics_functions.py lines 729-730, 1049 (98% → target ~100%).
• workflow.py line 65 (96% → target ~100%).

Phase 4 — Enforce coverage in CI (prevents regression)

1. Add --cov-fail-under=90 (or the value the team agrees on after Phase 2/3 land) to
   the test task in pixi.toml so coverage regressions fail CI instead of drifting
   silently, the way the original 66%-vs-92% gap in this issue did.
2. Confirm pixi run test and pixi run workflow_test both still pass with the new
   threshold.

Definition of done

• No duplicate test class names in tests/ (Phase 0)
• class_definitions.py and statistics_functions.py at 95%+ coverage
• Bespoke per-class mock accessors replaced by shared conftest.py fixtures
• --cov-fail-under threshold enforced in the test pixi task
• pixi run test and pixi run workflow_test green

[maxnutz]

Create cross validation between variables:

• Final Energy by carriers -> Final Energy
• Final energy by sector -> Final Energy