Fix duplicate test class, close coverage gaps, enforce coverage gate

[maxnutz]

Summary

• Phase 0: merge the byte-for-byte duplicate TestFinalEnergyByCarrierOil class in tests/test_statistics_functions.py (the second definition silently shadowed the first at import time — no tests were actually lost since both blocks were identical, but it was dead duplicate code). Added an ast-based regression guard (tests/test_no_duplicate_test_classes.py) that fails on any duplicate top-level test class name in tests/.
• Phase 1 (mock consolidation): assessed and skipped. The concrete case motivating this phase (two divergent _OilStatisticsAccessor mocks) turned out to be an exact duplicate, already resolved by Phase 0. The other 8 bespoke per-function mock accessors encode genuinely distinct pypsa.statistics call shapes, not copy-paste duplication, so consolidating them into one generic fixture wasn't pursued.
• Phase 2-3: added targeted unit tests closing coverage gaps in class_definitions.py (87% → 100%), statistics_functions.py (98% → 100%), and workflow.py (96% → 100%): Network_Processor.__init__ error branches, _get_network_config, _get_unit_from_common_definitions, calculate_variables_values multi-year aggregation, format_timestamps's TypeError fallback, and the module __main__ guard.
• Bug fix: while closing coverage, testing surfaced a real bug in _get_unit_from_common_definitions — the bracket-parsing check ran before the NaN check, so a NaN unit raised an undocumented TypeError instead of the documented KeyError, making the NaN-handling branch unreachable dead code. Fixed by reordering the checks (pypsa_validation_processing/class_definitions.py).
• Phase 4: enforce --cov-fail-under=90 on the test pixi task so coverage regressions fail CI instead of drifting silently.

Closes #32. The issue comment about cross-validation between Final Energy variables (by carrier / by sector) is out of scope here — tracked as separate follow-up work.

Test plan

• pixi run test — 224 passed, 100% coverage, --cov-fail-under=90 gate passes
• pixi run workflow_test — all 4 config setups (country/region × timeseries/year) run successfully, plus full test suite
• pixi run black --check on touched files
• tests/test_no_duplicate_test_classes.py passes, confirming no new name collisions

Summary by Sourcery

Remove duplicate test code, harden unit handling and configuration logic, and strengthen test coverage and CI coverage enforcement.

New Features:

• Add an AST-based guard that fails when duplicate top-level test classes exist in the tests suite.

Bug Fixes:

• Ensure _get_unit_from_common_definitions raises the documented KeyError for NaN units by checking for missing unit information before attempting to parse multi-unit strings.

Enhancements:

• Add tests covering Network_Processor initialization error paths, configuration discovery, aggregation behavior in calculate_variables_values, timestamp formatting fallbacks, and the workflow module’s main guard.
• Extend sectoral statistics tests to cover BEV charger efficiency warnings and empty residential/commercial oil demand handling.
• Improve unit conversion robustness with tests for multiple/common definition units and stricter error handling when units are missing or ambiguous.

Build:

• Tighten the test pixi task to run pytest with coverage on pypsa_validation_processing and fail when coverage drops below 90%.

[sourcery-ai]

Reviewer's Guide

Removes a silently shadowed duplicate test class, adds regression tests to guard against future duplicate test classes, extends tests to cover error/edge branches in Network_Processor, unit conversion, statistics functions, workflow main execution, and timestamp formatting, fixes _get_unit_from_common_definitions NaN-handling to raise the documented KeyError, and tightens CI by enforcing a 90% coverage minimum on the pytest task.

File-Level Changes

Change	Details	Files
Remove byte-for-byte duplicate Final_Energy_by_Carrier__Oil test class and add/extend statistics tests to cover additional branches and edge cases.	Delete the second, shadowing TestFinalEnergyByCarrierOil definition which was an exact duplicate of the first one. Add a test ensuring Final_Energy_by_Sector__Transportation warns and falls back to mean efficiency when BEV chargers have differing efficiencies. Extend residential/commercial statistics accessor and network test doubles to support an empty residential/commercial oil demand scenario, and add a test that Final_Energy_by_Sector__Residential_and_Commercial handles this without NaNs.	tests/test_statistics_functions.py
Increase coverage of Network_Processor.init, configuration handling, function dispatch, aggregation logic, and timestamp formatting behavior.	Add tests verifying Network_Processor.init raises FileNotFoundError or ValueError for missing network_results_path, missing or nonexistent definitions_path, and null model_name/scenario_name values. Add a test that _execute_function_for_variable prints a WARNING and returns None when the configured function name cannot be resolved on the target module. Introduce tests for _get_network_config covering multiple matching config files (INFO + first chosen), malformed YAML (WARNING + None), and no config file found (WARNING + None). Add aggregation tests for calculate_variables_values to raise RuntimeError when aggregate_per_year=False but a non-DataFrame result is returned, and to merge multiple investment years into a wide table when aggregate_per_year=True. Add a test that format_timestamps falls back to the non-"mixed" path when pd.to_datetime(format="mixed") raises TypeError, still producing localized datetime columns. Add a workflow-level test that running pypsa_validation_processing.workflow as main via runpy invokes main(), wires a Network_Processor with the CLI config path, and calls read_definitions, calculate_variables_values, and write_output_to_xlsx in sequence.	tests/test_network_processor.py tests/test_format_timestamps.py tests/test_workflow.py
Harden unit lookup and conversion helpers with additional validation and edge-case handling tests, and fix NaN unit handling order.	Change _get_unit_from_common_definitions so the NaN-unit check happens immediately after extracting the unit cell, raising the documented KeyError before attempting bracket parsing. Add tests verifying _get_unit_from_common_definitions warns and picks the first match when multiple definitions exist, raises RuntimeError when common_dsd is uninitialized, and raises KeyError when either the variable or unit column is missing. Add tests ensuring _get_unit_from_common_definitions correctly parses the first unit from a bracketed multi-unit string, falls back to 'TJ' when the multi-unit string is malformed, and raises KeyError when the unit is NaN. Add tests for _convert_units_to_common_definitions to raise ValueError when no unit is found for a variable or when multiple units are found, and to raise RuntimeError when a variable is missing from the common definitions.	pypsa_validation_processing/class_definitions.py tests/test_unit_conversion.py
Add an AST-based regression guard to prevent future duplicate top-level test classes in the test suite.	Introduce a test utility that walks all test_*.py files under tests/, parses them with ast, and fails if any top-level class name is defined more than once in the same module. Parameterize the guard test over all matching test files and report offending class names when duplicates are detected.	tests/test_no_duplicate_test_classes.py
Tighten CI coverage gate for the test task to prevent future coverage regressions.	Update the pixi test task to run pytest with coverage enabled on pypsa_validation_processing, show missing lines, and fail if total coverage drops below 90%.	pixi.toml

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#32	Eliminate the duplicate TestFinalEnergyByCarrierOil class and add a regression guard so no tests file in tests/ can define the same top-level test class name twice.	✅
#32	Increase test coverage for class_definitions.py, statistics_functions.py, and workflow.py to at least ~95% by adding tests for the specified error and edge branches, and enforce this via a coverage fail-under threshold in the pixi test task.	✅
#32	Consolidate bespoke per-function mock accessors in tests (e.g., custom StatisticsAccessor subclasses) onto shared fixtures from tests/conftest.py where possible, removing now-redundant bespoke mocks.	❌	The PR explicitly marks Phase 1 (mock consolidation) as assessed and skipped, arguing the remaining bespoke accessors encode distinct call shapes and are not duplication. No consolidation onto shared fixtures in tests/conftest.py is performed; bespoke mock classes remain.

Possibly linked issues

• Review testing routines #32: The PR fixes duplicate Oil tests, adds all requested coverage tests, introduces the duplicate-class guard, and enforces cov-fail-under.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The _setup_processor helper used in both TestGetNetworkConfig and TestCalculateVariablesValuesAggregation (and similar config/setup patterns in other tests) is nearly identical; consider factoring this into a shared helper or pytest fixture to reduce duplication and keep future changes to the setup logic in one place.
• Several new tests assert on exact printed WARNING/INFO strings from Network_Processor methods; consider centralizing these messages as constants or, better, switching to the logging module so tests can assert on log records instead of brittle stdout string matches.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `_setup_processor` helper used in both `TestGetNetworkConfig` and `TestCalculateVariablesValuesAggregation` (and similar config/setup patterns in other tests) is nearly identical; consider factoring this into a shared helper or pytest fixture to reduce duplication and keep future changes to the setup logic in one place.
- Several new tests assert on exact `print`ed WARNING/INFO strings from `Network_Processor` methods; consider centralizing these messages as constants or, better, switching to the logging module so tests can assert on log records instead of brittle stdout string matches.

## Individual Comments

### Comment 1
<location path="tests/test_network_processor.py" line_range="1627-1636" />
<code_context>
+class TestCalculateVariablesValuesAggregation:
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen aggregation test by asserting on actual merged values, not just column labels.

In `test_merges_multiple_investment_years_when_aggregated`, you currently only assert the presence of columns `"2020"` and `"2030"` and the single-row shape. Please also assert the actual cell values against the per-year outputs from `fake_execute` (e.g. `assert result.loc[("AT1", "MWh_el"), "2020"] == 2020.0` and similarly for 2030) to ensure the aggregation logic and reindexing are correct, not just the column labels.

Suggested implementation:

```python
        assert "2020" in result.columns
        assert "2030" in result.columns
        assert result.shape == (1, 2)

        # Assert merged values for each investment year to validate aggregation
        assert result.loc[("AT1", "MWh_el"), "2020"] == 2020.0
        assert result.loc[("AT1", "MWh_el"), "2030"] == 2030.0

```

If the test currently uses a different MultiIndex key than `("AT1", "MWh_el")`, or different numeric values from `fake_execute`, adjust the tuple and the expected numbers accordingly. Likewise, if the index is a simple Index (not MultiIndex), replace `("AT1", "MWh_el")` with the appropriate single index label used by the test.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Strengthen aggregation test by asserting on actual merged values, not just column labels.

In test_merges_multiple_investment_years_when_aggregated, you currently only assert the presence of columns "2020" and "2030" and the single-row shape. Please also assert the actual cell values against the per-year outputs from fake_execute (e.g. assert result.loc[("AT1", "MWh_el"), "2020"] == 2020.0 and similarly for 2030) to ensure the aggregation logic and reindexing are correct, not just the column labels.

Suggested implementation:

        assert "2020" in result.columns
        assert "2030" in result.columns
        assert result.shape == (1, 2)

        # Assert merged values for each investment year to validate aggregation
        assert result.loc[("AT1", "MWh_el"), "2020"] == 2020.0
        assert result.loc[("AT1", "MWh_el"), "2030"] == 2030.0

If the test currently uses a different MultiIndex key than ("AT1", "MWh_el"), or different numeric values from fake_execute, adjust the tuple and the expected numbers accordingly. Likewise, if the index is a simple Index (not MultiIndex), replace ("AT1", "MWh_el") with the appropriate single index label used by the test.