Skip to content

test(executor): add unit tests for wrap_executor and its helpers#782

Open
pmrv wants to merge 3 commits into
mainfrom
claude/gifted-meitner-kt6uww
Open

test(executor): add unit tests for wrap_executor and its helpers#782
pmrv wants to merge 3 commits into
mainfrom
claude/gifted-meitner-kt6uww

Conversation

@pmrv

@pmrv pmrv commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Motivation

A branch-coverage pass over tests/unit (pytest-cov, --cov-branch)
identified src/fleche/executor.py as the least covered module in the
package: 16.0% of statements, all 12 branches missed. Every other
src/fleche/* module was above 80%, most above 93%.

  16.0%    38 stmts    30 missing  br: 12/ 12  src/fleche/executor.py
  81.5%    21 stmts     3 missing  br:  6/  2  src/fleche/__main__.py
  83.3%    12 stmts     2 missing  br:  0/  0  src/fleche/_attrs.py
  86.7%    13 stmts     2 missing  br:  2/  0  src/fleche/__init__.py
  91.4%   369 stmts    26 missing  br: 82/ 13  src/fleche/remote.py
  ... (rest ≥ 91%) ...

The gap comes from wrap_executor and its two helpers
(_is_fleche_function, _split_submit_kwargs) only being reached
end-to-end through the process-pool tests in
tests/integration/test_parallel_execution.py — the module itself had
no unit-level test file. That makes the executor branches a soft spot in
regression coverage: each subprocess spin-up is slow, and none of the
branches (already-wrapped shortcut; signature-parse fallback;
cache-hit-skips-submit) reads like something the integration suite is
guaranteed to keep exercising as the surrounding code evolves.

What this changes

Adds tests/unit/test_executor.py, a 15-test file that drives the
module in isolation with three lightweight executor stubs:

  • _StubExecutor — records what gets submitted; runs the callable
    synchronously in a completed Future.
  • _NeverSubmitExecutor — raises on any submit call, so a
    cache-hit branch that fails to short-circuit fails loudly.
  • _KeywordOnlyExecutor — declares a keyword-only resources=
    param, modelling the HPC-executor shape (executorlib / dask) that
    motivated _split_submit_kwargs.

Test groups:

Class Behaviour pinned
TestIsFlecheFunction plain callable, fleche-decorated callable, and non-SimpleNamespace .fleche attribute (branch coverage for isinstance check)
TestSplitSubmitKwargs no-keyword-only pass-through, keyword-only split, empty-kwargs edge case, inspect.signature TypeError fallback (via unittest.mock.patch)
TestWrapExecutor returns same instance, marks submit._fleche_wrapped, idempotent second wrap, non-fleche BoundWrapper.bind, already-BoundWrapper passthrough (PR #724), cache-hit skips submit, cache-miss binds and forwards, keyword-only kwargs partitioned correctly

Coverage impact

Measured with pytest tests/unit --cov=fleche --cov-branch:

  • src/fleche/executor.py in isolation goes from 16% / 0% branch
    to 100% / 100% branch (0 missing lines, 0 missing branches) — no
    other module's coverage regresses.
  • Overall tests/unit branch coverage was 94.0% before; the 30 new
    statements + 12 new branches lift it further, with no offsetting
    drop elsewhere.

Test plan

  • pytest tests/unit/test_executor.py -v — all 15 pass locally.
  • pytest tests/unit/test_executor.py --cov=fleche shows
    src/fleche/executor.py at 100% statements + 100% branches.
  • The three new stub classes don't depend on any optional extra
    (no cloudpickle / no executorlib / no ssh path) — the file runs
    on the minimum-deps CI matrix.

Generated by Claude Code

The integration suite exercises wrap_executor end-to-end through real
thread and process pools, but the module's own control flow —
_is_fleche_function, _split_submit_kwargs (both branches of the
inspect.signature try/except), the already-wrapped shortcut, and the
cache-hit/miss branches inside the patched submit — was not covered by
any unit-level test. Coverage of src/fleche/executor.py from tests/unit
was 16%; with these 15 in-process tests it is 100% (statements and
branches).

The tests use lightweight executor stubs (_StubExecutor,
_NeverSubmitExecutor, _KeywordOnlyExecutor) so each branch runs without
paying for a subprocess spin-up.
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🤖 CI failure summary for da7098c on claude/gifted-meitner-kt6uww

Summary
Three executor tests are failing across Python 3.11, 3.12, and 3.13. The tests expect BoundWrapper instances to be submitted to the executor, but wrapped functions are being double-wrapped instead.

Root cause
The wrap_executor function is wrapping callables that are already BoundWrapper instances. The tests show:

  • test_non_fleche_callable_is_bound_before_submission: expects isinstance(submitted_func, BoundWrapper) but gets a nested wrapper
  • test_already_bound_wrapper_is_forwarded_as_is: expects the same BoundWrapper object but gets a new one wrapping the original
  • test_cache_miss_binds_and_submits: similar double-wrapping issue
AssertionError: assert False
 +  where False = isinstance(BoundWrapper(...), BoundWrapper)

The problem is that wrap_executor should skip wrapping if the callable is already a BoundWrapper, but it's currently wrapping it again.

Suggested fix
In the wrap_executor function (likely in src/fleche/executor.py), add an isinstance check before wrapping:

  • Check if the callable is already a BoundWrapper
  • If yes, pass it through unchanged
  • If no, proceed with binding as normal

This ensures idempotence so that pre-bound wrappers aren't double-wrapped.


Generated by Claude Haiku 4.5.

@pmrv

pmrv commented Jul 23, 2026

Copy link
Copy Markdown
Owner Author

@claude ^

@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @pmrv's task in 6m 48s —— View job


Root cause found — fixed a test-isolation bug, not wrap_executor itself

  • Gather context (PR diff, prior CI-failure comment)
  • Reproduce/confirm the failure (read the actual CI job logs, not just the bot's summary)
  • Determine root cause
  • Implement fix
  • Push

The Haiku bot's diagnosis was wrong. src/fleche/executor.py already has the isinstance(func, state.BoundWrapper) short-circuit (added in #724, well before this branch was cut) — it's untouched by this PR, which only adds tests/unit/test_executor.py.

Reading the actual job log (not just the summary) shows the real signature of the bug:

E       AssertionError: assert False
E        +  where False = isinstance(BoundWrapper(func=<function ...plain...>, ...), BoundWrapper)

isinstance returning False against an object whose repr() is literally BoundWrapper(...) is the classic symptom of two distinct class objects with the same name — not a double-wrap bug (the apparent nested-BoundWrapper repr in the second failure is a downstream effect of that, not the cause).

Tracing it down: tests/unit/config/test_config.py has a restore_fleche_state fixture (used by 3 tests) that calls importlib.reload(fleche.state) to simulate fresh-process config loading. That gives fleche.state a brand-new BoundWrapper class object. But fleche/__init__.py does from .state import BoundWrapper at package-import time, so fleche.BoundWrapper keeps pointing at the pre-reload class forever after — while src/fleche/executor.py does from . import state and looks up state.BoundWrapper fresh on every call, so it sees the new class. Since tests/unit/config/ sorts before tests/unit/test_executor.py alphabetically, every full-suite run silently diverges the two references before the executor tests even start, breaking any isinstance check that spans the public API (fleche.BoundWrapper) and the internal one (executor.py's state.BoundWrapper).

Fix (tests/unit/config/test_config.py): the restore_fleche_state fixture now also reloads the fleche package itself after reloading fleche.state, resyncing fleche.BoundWrapper (and the other re-exports) with whatever fleche.state currently holds. fleche/__init__.py has no risky side effects to re-run (just rebinding names), so this is safe. Pushed as a793292.

Note: I wasn't able to run pytest locally in this sandbox (Python execution requires interactive approval this environment doesn't grant), so this fix is verified by log analysis and code tracing rather than a local run — worth confirming the new CI run goes green. If you'd like me to also run the suite here, granting python/pytest execution in --allowedTools would let me do that directly next time.
·

…ate)

Reloading fleche.state in place (in the restore_fleche_state fixture used
by three config tests) gives it fresh class objects, e.g. a new
BoundWrapper, but fleche/__init__.py re-exports the same names via
`from .state import ...` bound at package-import time. Without also
reloading fleche itself, fleche.BoundWrapper kept pointing at the
pre-reload class while executor.py's `state.BoundWrapper` (looked up
fresh via `from . import state`) picked up the new one. That silent
divergence broke the new isinstance(func, BoundWrapper) checks in
tests/unit/test_executor.py for every test running after the config
tests in the same session, since both classes have the same repr.

Co-authored-by: Marvin Poul <2719909+pmrv@users.noreply.github.com>
@pmrv

pmrv commented Jul 23, 2026

Copy link
Copy Markdown
Owner Author

@claude still broken

@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @pmrv's task in 4m 0s —— View job


Fixed — real root cause this time

  • Gather context (PR diff, prior fix, CI-failure history)
  • Get latest CI run logs
  • Determine root cause
  • Implement fix
  • Push

Why the previous fix (a793292) didn't hold: it reloaded fleche/__init__.py after reloading fleche.state, to resync fleche.BoundWrapper with the brand-new BoundWrapper class object that importlib.reload(fleche.state) creates. But tests/unit/test_executor.py:21 does from fleche import BoundWrapper at module-collection time — before any test body runs. Resyncing fleche.BoundWrapper later doesn't touch that already-bound name; it stays frozen to the original (pre-reload) class forever. Once the config tests reload fleche.state later in the run, executor.py's live state.BoundWrapper lookup (src/fleche/executor.py:92) sees the new class while test_executor.py's captured name still points to the old one → same three isinstance failures as before, byte-for-byte.

Actual fix (tests/unit/config/test_config.py): stop letting a second BoundWrapper class escape at all. _reload_fleche_state() now reloads fleche.state and then restores fleche.state.BoundWrapper = original_bound_wrapper. Because importlib.reload re-executes a module inside its existing __dict__, the restored class's bind() classmethod still resolves _CACHE/_METADATA against the freshly reloaded ContextVars — only the class identity is preserved, not its stale config. Every reference taken anywhere (before or after the reload — this file, test_executor.py, tests/integration/test_parallel_execution.py, user code) now points at the same class, so no downstream isinstance check can ever diverge. This is more robust than the previous approach, which only patched the one path (fleche.__init__) that happened to be involved in this specific failure.

Verification note: as with the previous pass, I could not execute pytest/python directly in this sandbox (execution requires interactive approval this environment doesn't grant) — the fix is verified by tracing importlib.reload semantics and the exact import order in the failing log, not a local test run. Worth confirming the new CI run goes green; granting python/pytest in --allowedTools would let me verify directly next time.
· branch

The prior fix (a793292) reloaded fleche/__init__.py to resync
fleche.BoundWrapper with the new class object created by
importlib.reload(fleche.state). But tests/unit/test_executor.py does
`from fleche import BoundWrapper` at module-collection time, before
any test runs, so its reference stays frozen to the pre-reload class
regardless of any later resync -- reproducing the same isinstance
failures.

Instead, restore the original BoundWrapper class object onto the
reloaded module. Since importlib.reload re-executes a module in its
existing __dict__, the restored class's bind() classmethod still
resolves the freshly reloaded _CACHE/_METADATA ContextVars -- only the
class identity is preserved, so every reference taken before or after
the reload (in this file, test_executor.py, or elsewhere) keeps
pointing at the same class.

Co-authored-by: Marvin Poul <2719909+pmrv@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants