Skip to content

fix(tests,constraints): resolve test failures from bad constructor call, rcParams leak, and scalar Quantity indexing#142

Open
pankaj-bind wants to merge 1 commit into52North:mainfrom
pankaj-bind:fix/test-failures-constructor-rcparams-scalar-quantity
Open

fix(tests,constraints): resolve test failures from bad constructor call, rcParams leak, and scalar Quantity indexing#142
pankaj-bind wants to merge 1 commit into52North:mainfrom
pankaj-bind:fix/test-failures-constructor-rcparams-scalar-quantity

Conversation

@pankaj-bind
Copy link
Contributor

@pankaj-bind pankaj-bind commented Feb 20, 2026

Related Issue / Discussion:

Fixes Issue #141


Changes:

  • Modified tests/test_route_postprocessing.py removed incorrect boat_speed positional arg from RoutePostprocessing instantiation
  • Modified tests/test_direct_power_method.py wrapped text.usetex assignment in try/finally to restore state after test
  • Modified WeatherRoutingTool/constraints/route_postprocessing.py added isscalar guard in calculate_timsestamp() before indexing speed

Further Details:

Summary:

Three independent bugs were causing test failures on a clean pytest tests/ run.

1. tests/test_route_postprocessing.py
RoutePostprocessing was being instantiated with boat_speed as a third positional arg. The constructor signature is (min_fuel_route, boat, db_engine=None) speed is already derived internally via set_data() from ship_params_per_step.speed. This caused TypeError: multiple values for argument 'db_engine' on all tests in
that class.

2. tests/test_direct_power_method.py
test_wind_coeff sets plt.rcParams['text.usetex'] = True globally and never restores it. Any test running after this with math-style labels fails with RuntimeError: latex is not installed. Wrapped in try/finally to restore the
original value.

3. WeatherRoutingTool/constraints/route_postprocessing.py
calculate_timsestamp() unconditionally indexes speed[previous_step_index], which raises TypeError when speed is a scalar astropy.units.Quantity. Added an isscalar guard 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

@pankaj-bind
Copy link
Contributor Author

pankaj-bind commented Feb 21, 2026

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.
@kdemmich @MartinPontius

Copilot AI review requested due to automatic review settings March 3, 2026 13:53
@pankaj-bind pankaj-bind force-pushed the fix/test-failures-constructor-rcparams-scalar-quantity branch from dc372ee to 95edb09 Compare March 3, 2026 13:53
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in tests/test_route_postprocessing.py to match the constructor signature (avoid positional-arg collision with db_engine).
  • Wrap plt.rcParams['text.usetex'] modification in tests/test_direct_power_method.py with try/finally to restore global state after the test.
  • Add an isscalar guard in RoutePostprocessing.calculate_timsestamp() to avoid indexing into scalar Quantity values.

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
@pankaj-bind pankaj-bind force-pushed the fix/test-failures-constructor-rcparams-scalar-quantity branch from 95edb09 to 397e6ce Compare March 3, 2026 14:17
@pankaj-bind
Copy link
Contributor Author

Update: Rebased + improved

Rebased on latest main. The boat_speed constructor fix in test_route_postprocessing.py was already applied upstream by @MartinPontius (Fix bug in tests/test_route_postprocessing.py), so it absorbed cleanly during rebase.

Changes in this update:

  • Replaced try/finally rcParams cleanup with plt.rc_context() - cleaner, idiomatic matplotlib
  • Added test_recalculate_starttime_scalar_speed - covers the scalar speed branch in calculate_timsestamp()

@kdemmich kdemmich added the bug Something isn't working label Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants