fix(tests,constraints): resolve test failures from bad constructor call, rcParams leak, and scalar Quantity indexing#142
Conversation
|
The lint (3.13) CI job failure is unrelated to this change, it's caused by an expired/missing GL_TOKEN secret needed to clone the private maripower dependency. |
dc372ee to
95edb09
Compare
There was a problem hiding this comment.
Pull request overview
Resolves three independent pytest tests/ failures reported in Issue #141 by fixing a bad RoutePostprocessing constructor call in tests, preventing a global Matplotlib rcParams leak across tests, and handling scalar astropy.units.Quantity speeds in route postprocessing timestamp calculations.
Changes:
- Fix
RoutePostprocessing(...)instantiation intests/test_route_postprocessing.pyto match the constructor signature (avoid positional-arg collision withdb_engine). - Wrap
plt.rcParams['text.usetex']modification intests/test_direct_power_method.pywithtry/finallyto restore global state after the test. - Add an
isscalarguard inRoutePostprocessing.calculate_timsestamp()to avoid indexing into scalarQuantityvalues.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/test_route_postprocessing.py |
Updates test setup to call RoutePostprocessing with correct arguments (prevents constructor argument collision). |
tests/test_direct_power_method.py |
Restores text.usetex after the manual plotting test to avoid contaminating subsequent tests. |
WeatherRoutingTool/constraints/route_postprocessing.py |
Prevents TypeError when speed is a scalar Quantity by avoiding unconditional indexing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ll, rcParams leak, and scalar Quantity indexing
95edb09 to
397e6ce
Compare
|
Update: Rebased + improved Rebased on latest Changes in this update:
|
Related Issue / Discussion:
Fixes Issue #141
Changes:
tests/test_route_postprocessing.pyremoved incorrectboat_speedpositional arg fromRoutePostprocessinginstantiationtests/test_direct_power_method.pywrappedtext.usetexassignment in try/finally to restore state after testWeatherRoutingTool/constraints/route_postprocessing.pyaddedisscalarguard incalculate_timsestamp()before indexingspeedFurther Details:
Summary:
Three independent bugs were causing test failures on a clean
pytest tests/run.1. tests/test_route_postprocessing.py
RoutePostprocessingwas being instantiated withboat_speedas a third positional arg. The constructor signature is(min_fuel_route, boat, db_engine=None)speed is already derived internally viaset_data()fromship_params_per_step.speed. This causedTypeError: multiple values for argument 'db_engine'on all tests inthat class.
2. tests/test_direct_power_method.py
test_wind_coeffsetsplt.rcParams['text.usetex'] = Trueglobally and never restores it. Any test running after this with math-style labels fails withRuntimeError: latex is not installed. Wrapped in try/finally to restore theoriginal value.
3. WeatherRoutingTool/constraints/route_postprocessing.py
calculate_timsestamp()unconditionally indexesspeed[previous_step_index], which raisesTypeErrorwhenspeedis a scalarastropy.units.Quantity. Added anisscalarguard before indexing.PR Checklist:
In the context of this PR, I:
Please consider that PRs which do not meet the requirements specified in the checklist will not be evaluated. Also, PRs with no activities will be closed after a reasonable amount of time.
@kdemmich @MartinPontius