Consolidate test helpers and tighten loose assertions#39
Merged
Conversation
- Add ts(), waste(), assert_on_blocks(), assert_off_blocks() to root conftest.py - Remove duplicate _ts()/_waste() from 5 files, replace timesteps_3/4 fixtures - Replace 12 inline block-verification loops with shared numpy-based helpers - Tighten ~16 directional assertions (> X) with exact expected values - Add pythonpath=["tests"] to pytest config for cross-directory imports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughCentralizes test utilities by adding ts/waste and block-assertion helpers in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/math/test_status.py (1)
896-900:⚠️ Potential issue | 🟡 MinorLocal variable
tsshadows the module-level import in both half-hour timestep testsIn
test_min_uptime_with_half_hour_timesteps(line 898) andtest_max_uptime_with_half_hour_timesteps(line 938), a local listtsis assigned and immediately passed tooptimize. Within both methodstsnow refers to the local list, silently hiding the conftest helper. This works today but will cause a subtleTypeError(calling a list as a function) if someone later writests(n)inside either test.🐛 Proposed fix (applies to both methods)
- ts = [datetime(2020, 1, 1, h, m) for h in range(4) for m in (0, 30)] + timesteps_30min = [datetime(2020, 1, 1, h, m) for h in range(4) for m in (0, 30)] result = optimize( - ts, + timesteps_30min,And the same rename in the second test at line 938 (
range(3)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/math/test_status.py` around lines 896 - 900, The local variable ts in both tests (test_min_uptime_with_half_hour_timesteps and test_max_uptime_with_half_hour_timesteps) shadows the module-level ts helper; rename the local list (e.g., to ts_list or timestamps_half_hour) and update its usages passed to optimize so the module-level ts remains callable; ensure you rename the second occurrence where range(3) is used as well so all local references are consistent.
🧹 Nitpick comments (5)
tests/test_types.py (1)
9-16:tsfixture is defined but never consumed — consider removing it.No test method in this file has
tsas a parameter; every method that needs aDatetimeIndexconstructs one locally (e.g. line 49). The_tsalias import at line 9 exists solely to populate this dead fixture.♻️ Proposed cleanup
-from conftest import ts as _ts ... -@pytest.fixture -def ts(): - return pd.DatetimeIndex(_ts(3)) - - class TestNormalizeTimesteps:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_types.py` around lines 9 - 16, The file defines an unused pytest fixture named "ts" and an unused import alias "_ts"; remove the dead fixture and the unused import to clean up the test module: delete the "from conftest import ts as _ts" import and the "@pytest.fixture def ts(): return pd.DatetimeIndex(_ts(3))" block so tests continue to create DatetimeIndex locally (no changes to as_dataarray/compute_dt/normalize_timesteps required); run the test suite to confirm nothing else depended on the fixture.tests/conftest.py (2)
8-9: Unnecessary split betweenTYPE_CHECKINGguard and in-function import — prefer a plain top-level import
numpyis a mandatory test dependency, not an optional one. TheTYPE_CHECKINGguard (line 8–9) combined with the deferredimport numpy as npinside_block_lengths(line 29) is the right pattern for optional deps but adds needless indirection here. A single unconditional top-level import is simpler and equivalent.♻️ Proposed fix
from __future__ import annotations from datetime import datetime -from typing import TYPE_CHECKING +import numpy as np from fluxopt import Flow, Port -if TYPE_CHECKING: - import numpy as npThen remove the local
import numpy as npon line 29 inside_block_lengths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 8 - 9, Remove the unnecessary TYPE_CHECKING guard and make numpy a top-level import: replace the conditional "if TYPE_CHECKING: import numpy as np" with a normal "import numpy as np" at module scope, and delete the local "import numpy as np" inside the _block_lengths function so all uses reference the top-level numpy import.
17-19:wastedocstring is missing theArgssection
busis a non-obvious parameter name; anArgsentry would document its purpose.♻️ Proposed fix
def waste(bus: str) -> Port: - """Free-disposal port that absorbs excess on *bus* at zero cost.""" + """Free-disposal port that absorbs excess on *bus* at zero cost. + + Args: + bus: Bus identifier to absorb excess flow from. + """ return Port(f'_waste_{bus}', exports=[Flow(bus=bus)])As per coding guidelines, public functions with parameters should use Google-style docstrings with an
Argssection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 17 - 19, The docstring for the public function waste lacks a Google-style Args section describing the bus parameter; update the waste function's docstring to include an "Args" entry that explains that bus is the name/identifier of the energy/electricity carrier being absorbed (e.g., a string representing the bus/commodity) and any expectations (type: str) and behavior, while keeping the brief description and mention of returned Port (constructed with Port and Flow) intact.tests/math/test_contributions.py (2)
197-208: Consider deriving the expected investment cost instead of using a literal
5000.0is not self-documenting; a reader must mentally tracemin_size(50) * effects_per_size(100)to verify it. Since the solver picksmin_sizeas the optimal size (demand is exactly 50 MW), this can be expressed directly:♻️ Proposed refactor
- assert grid_inv == pytest.approx(5000.0, abs=1e-6) + invest_size = 50.0 # min_size; solver picks minimum to meet 50 MW demand + assert grid_inv == pytest.approx(invest_size * 100, abs=1e-6)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/math/test_contributions.py` around lines 197 - 208, Replace the hard-coded 5000.0 assertion with a derived expected value so the test documents how the solver arrived at that number: compute expected_investment as min_size * effects_per_size (e.g., 50 * 100 per the model) then assert grid_inv == pytest.approx(expected_investment, abs=1e-6); update the test around result.effect_contributions(), grid_inv and demand_inv to use the computed expected value (define min_size and effects_per_size as constants in the test if they aren’t already) and keep the final sum-vs-solver assertion unchanged.
288-293: Derive22.6inline for readabilityThe tightened assertion replaces a loose check but the magic number
22.6obscures the two cost components it contains. Expressing the computation explicitly makes the test self-documenting:♻️ Proposed refactor
- assert grid_cost == pytest.approx(22.6, abs=1e-6) + expected_flow_cost = (50 + 80 + 60) * 0.04 # 7.6 + expected_running_cost = 3 * 5.0 # 15.0, running all 3 timesteps + assert grid_cost == pytest.approx(expected_flow_cost + expected_running_cost, abs=1e-6)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/math/test_contributions.py` around lines 288 - 293, The test uses a magic number 22.6 for grid_cost; instead derive that expected value by summing the actual cost components from the contrib dataset so the test is self-documenting: locate the contrib object and the grid_cost variable and replace the literal 22.6 with an explicit computation that selects and sums the two cost parts for contributor='grid(elec)' (e.g., the fixed and running cost entries) and assert pytest.approx(expected_sum, abs=1e-6) against grid_cost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/conftest.py`:
- Around line 84-91: The docstring for assert_off_blocks incorrectly states that
"values < 0.5 are 'off'"; update it to match the implementation in
_block_lengths which treats off as "not (value > 0.5)" (i.e., value ≤ 0.5).
Change the on parameter description in assert_off_blocks to say "values ≤ 0.5
are 'off'" (or "<= 0.5" if you prefer ASCII) so the documentation matches the
logic in _block_lengths and any references to skip_leading/on semantics remain
consistent.
- Around line 12-14: The ts(n) helper currently constructs datetimes with
datetime(2024,1,1,h) which raises ValueError for n>24; change ts to build the
sequence by starting at datetime(2024,1,1) and adding
datetime.timedelta(hours=i) for i in range(n) so it supports arbitrarily many
hourly steps, and update the docstring to include a Google-style Args section
documenting the n parameter and return value; locate and modify the ts function
to implement these changes.
---
Outside diff comments:
In `@tests/math/test_status.py`:
- Around line 896-900: The local variable ts in both tests
(test_min_uptime_with_half_hour_timesteps and
test_max_uptime_with_half_hour_timesteps) shadows the module-level ts helper;
rename the local list (e.g., to ts_list or timestamps_half_hour) and update its
usages passed to optimize so the module-level ts remains callable; ensure you
rename the second occurrence where range(3) is used as well so all local
references are consistent.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 8-9: Remove the unnecessary TYPE_CHECKING guard and make numpy a
top-level import: replace the conditional "if TYPE_CHECKING: import numpy as np"
with a normal "import numpy as np" at module scope, and delete the local "import
numpy as np" inside the _block_lengths function so all uses reference the
top-level numpy import.
- Around line 17-19: The docstring for the public function waste lacks a
Google-style Args section describing the bus parameter; update the waste
function's docstring to include an "Args" entry that explains that bus is the
name/identifier of the energy/electricity carrier being absorbed (e.g., a string
representing the bus/commodity) and any expectations (type: str) and behavior,
while keeping the brief description and mention of returned Port (constructed
with Port and Flow) intact.
In `@tests/math/test_contributions.py`:
- Around line 197-208: Replace the hard-coded 5000.0 assertion with a derived
expected value so the test documents how the solver arrived at that number:
compute expected_investment as min_size * effects_per_size (e.g., 50 * 100 per
the model) then assert grid_inv == pytest.approx(expected_investment, abs=1e-6);
update the test around result.effect_contributions(), grid_inv and demand_inv to
use the computed expected value (define min_size and effects_per_size as
constants in the test if they aren’t already) and keep the final sum-vs-solver
assertion unchanged.
- Around line 288-293: The test uses a magic number 22.6 for grid_cost; instead
derive that expected value by summing the actual cost components from the
contrib dataset so the test is self-documenting: locate the contrib object and
the grid_cost variable and replace the literal 22.6 with an explicit computation
that selects and sums the two cost parts for contributor='grid(elec)' (e.g., the
fixed and running cost entries) and assert pytest.approx(expected_sum, abs=1e-6)
against grid_cost.
In `@tests/test_types.py`:
- Around line 9-16: The file defines an unused pytest fixture named "ts" and an
unused import alias "_ts"; remove the dead fixture and the unused import to
clean up the test module: delete the "from conftest import ts as _ts" import and
the "@pytest.fixture def ts(): return pd.DatetimeIndex(_ts(3))" block so tests
continue to create DatetimeIndex locally (no changes to
as_dataarray/compute_dt/normalize_timesteps required); run the test suite to
confirm nothing else depended on the fixture.
- Fix ts() to use timedelta (supports n>24), add Args docstrings - Move numpy to top-level import, remove TYPE_CHECKING guard - Fix assert_off_blocks docstring: "<= 0.5" matches implementation - Rename local ts → half_hour_ts to avoid shadowing module-level helper - Remove unused ts fixture and _ts import from test_types.py - Replace magic numbers in test_contributions.py with derived expressions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/math/test_status.py (1)
19-65: Optional: tighten the objective assertion to match the docstring's stated total.The docstring explicitly calculates
Total: 50 + 60 = 110, yet noassert_allclose(result.objective, …)is added. Since the cost is fully determined (fixed size, deterministic dispatch), this is straightforward to lock in and consistent with the PR's tightening goal.✨ Suggested addition
assert rates[1] >= 60.0 - 1e-5 + assert_allclose(result.objective, 110.0, rtol=1e-5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/math/test_status.py` around lines 19 - 65, Add an assertion that the optimization objective equals the docstring’s computed total (110) to make the test deterministic: after calling optimize(...) and before finishing the test, assert that result.objective (or result.objective_value if that API is used) is approximately 110.0 with a small tolerance (e.g. atol=1e-5) so the test verifies the total cost stated in the docstring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/conftest.py`:
- Around line 89-96: The docstring for assert_off_blocks correctly states that
values <= 0.5 are considered "off" and requires no change; leave the docstring
in tests/conftest.py (the assert_off_blocks function) as-is and proceed with
approval/merge.
- Around line 10-17: The ts function correctly generates n hourly timesteps
starting at 2024-01-01 using datetime and timedelta, and the Google-style Args
docstring is present; no code changes are required—leave the ts function (named
ts) and its implementation/Docstring as-is.
---
Nitpick comments:
In `@tests/math/test_status.py`:
- Around line 19-65: Add an assertion that the optimization objective equals the
docstring’s computed total (110) to make the test deterministic: after calling
optimize(...) and before finishing the test, assert that result.objective (or
result.objective_value if that API is used) is approximately 110.0 with a small
tolerance (e.g. atol=1e-5) so the test verifies the total cost stated in the
docstring.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_ts,_waste, on-block verification loops) into shared functions in rootconftest.pytimesteps_3/timesteps_4fixtures withts(n)function calls across all test filesassert_on_blocks()/assert_off_blocks()helpers> X) with exact expected values where computablepythonpath=["tests"]to pytest config for cross-directory importsChanges
Helpers consolidated into
tests/conftest.pyts(n)nhourly timesteps starting 2024-01-01waste(bus)assert_on_blocks(on, min_length, max_length)assert_off_blocks(on, min_length, max_length, skip_leading)Assertions tightened
Replaced directional checks (
assert obj > X) with exact values in:test_status.py— 6 assertions (objectives: 140, 1020, 200, 140, 240, 120)test_flow_status.py— 1 assertion (60.0)test_combinations.py— 1 assertion (140.0)test_custom.py— 1 assertion (150.0)test_end_to_end.py— 1 assertion (exact formula)test_contributions.py— 4 assertions (5000, 22.6, formula, computed)test_sizing.py— 2 left directional (size not uniquely determined without per-size cost)Files modified (20)
pyproject.toml,tests/conftest.py,tests/test_data.py,tests/test_types.py,tests/math_port/conftest.py, and 15 test files.Type of Change
Testing
ruff checkcleanSummary by CodeRabbit
Chores
Tests