Skip to content

Add lexically-scoped hooks with Depends()#39

Open
thejchap wants to merge 6 commits intomainfrom
hooks-and-depends
Open

Add lexically-scoped hooks with Depends()#39
thejchap wants to merge 6 commits intomainfrom
hooks-and-depends

Conversation

@thejchap
Copy link
Copy Markdown
Owner

Summary

  • Implements proposal 5 from fixtures/hooks #36: @before_each, @before_all, @after_each, @after_all, @wrap_each, @wrap_all decorators with FastAPI-style Depends() for typed, explicit dependency injection
  • Scope inferred from lexical position relative to describe blocks (no conftest.py, no session scope)
  • Depends() uses overloaded typing so type checkers see the correct return type
  • Full pipeline: Rust AST discovery → hook metadata → Python worker execution

What's included

Python API (python/tryke/hooks.py):

  • Six hook decorators that stamp dunder attributes
  • Depends() with overloaded typing for type-checker compatibility
  • DependencyResolver: DAG resolution with caching and cycle detection
  • HookExecutor: orchestrates before → wrap setup → test → wrap teardown → after

Rust discovery (crates/tryke_discovery/):

  • is_tryke_hook_decorator() recognizes all six hook decorators in AST
  • extract_depends_from_params() parses Depends(name) in parameter defaults
  • parse_tests_from_source() now returns ParsedFile (tests + hooks)

Rust types (crates/tryke_types/):

  • HookType enum, HookItem struct, ParsedFile struct

Worker integration:

  • register_hooks JSON-RPC method sends hook metadata per module
  • Worker builds HookExecutor from registered hooks
  • run_test now passes groups for scope resolution

What's NOT included (follow-up work)

  • Scheduling constraints for _all hooks (Step 6)
  • Error handling/propagation for hook failures (Step 7)
  • Async hook support in executor
  • Cross-describe before_all sharing

Test plan

  • 26 unit tests for decorators, Depends, resolver, executor
  • 3 end-to-end tests verifying hooks work through the full pipeline
  • All 126 Python tests pass
  • All 403 Rust tests pass
  • cargo clippy --workspace --all-targets --all-features -- -D warnings clean
  • uvx prek run -a passes

🤖 Generated with Claude Code

thejchap and others added 5 commits March 29, 2026 22:22
Implements @before_each, @before_all, @after_each, @after_all,
@wrap_each, @wrap_all decorators with FastAPI-style Depends() for
typed, explicit dependency injection. Scope is inferred from
lexical position relative to describe blocks.

Phases completed:
- Python decorators + Depends() with overloaded typing
- Rust types: HookType, HookItem, ParsedFile
- Rust discovery: AST hook detection + Depends() extraction
- Changed parse_tests salsa function to return ParsedFile
- Python DependencyResolver + HookExecutor runtime engine
- Worker integration: register_hooks RPC + hook orchestration
- Full end-to-end pipeline from discovery to execution

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- DistMode::Test upgrades to File/Group when _all hooks present
- Teardown/cleanup always runs even when test raises (try/finally)
- 3 new scheduling constraint tests, 3 new error handling tests

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WorkerPool::new only added the project root to PYTHONPATH, but the
tryke Python package lives at <root>/python/tryke/ (declared via
python-source = "python" in pyproject.toml). Without a venv, the
worker subprocess couldn't import tryke.

Now adds root/python to PYTHONPATH when the directory exists.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- 3 tests verify before_all instance is created once and cached
  across multiple tests (call count stays at 1)
- 2 tests verify Depends() chains: config → db/cache → service,
  with before_all reused and before_each fresh per test
- 1 test verifies describe-scoped hook depending on module-scoped
  before_all via Depends()

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- concepts/concurrency.md: new "Hooks and scheduling" section explaining
  how _all hooks constrain DistMode and what each combination means
  for parallelism
- guides/writing-tests.md: comprehensive hooks guide covering all six
  decorators, Depends() for typed state passing, scoping, composability,
  and execution order
- migration.md: replace "No fixtures (yet)" with pytest fixture → tryke
  hooks migration example
- reference/api.md: add hook decorator and Depends API entries

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

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

Implements lexically-scoped lifecycle hooks and FastAPI-style Depends() dependency injection across the full Rust discovery → runner scheduling → Python worker execution pipeline, plus docs and tests.

Changes:

  • Added Python hook API (before_*, after_*, wrap_*) + Depends() and a resolver/executor to orchestrate hook execution around tests.
  • Extended Rust discovery/types/protocol to discover hook metadata and send it to the Python worker via JSON-RPC (register_hooks) and to pass groups to run_test.
  • Updated runner scheduling to account for _all hooks and attached hook metadata to work units; added docs + unit/e2e tests.

Reviewed changes

Copilot reviewed 23 out of 24 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
tests/test_hooks_e2e.py New end-to-end coverage for hooks/Depends through the full pipeline
tests/test_hooks.py New unit tests for decorators, Depends typing, resolver, and executor behaviors
python/tryke/hooks.py New hooks/Depends implementation: decorators, dependency resolver, hook executor
python/tryke/worker.py Worker integration: hook registration, executor caching, groups passed to test run
python/tryke/init.py Re-export hooks + Depends from the top-level tryke package
docs/reference/api.md API reference additions for hooks and Depends
docs/migration.md Migration guide updates describing fixtures → hooks + Depends
docs/guides/writing-tests.md User guide section describing hook usage, scoping, and execution order
docs/concepts/concurrency.md Documentation for how _all hooks affect scheduling/distribution modes
crates/tryke_types/src/lib.rs New Rust types: HookType, HookItem, ParsedFile for discovery output
crates/tryke_discovery/src/lib.rs Hook decorator + Depends(...) extraction and returning hooks alongside tests
crates/tryke_discovery/src/discoverer.rs Surface discovered hooks via Discoverer::hooks() and adjust parsing API
crates/tryke_discovery/src/db.rs Salsa tracked parse_tests now returns ParsedFile instead of Vec<TestItem>
crates/tryke_runner/src/protocol.rs Protocol extensions: groups in RunTestParams, plus HookWire and RegisterHooksParams
crates/tryke_runner/src/worker.rs WorkerProcess sends groups and supports register_hooks RPC
crates/tryke_runner/src/schedule.rs WorkUnit now carries hooks; new partition_with_hooks w/ mode upgrading logic
crates/tryke_runner/src/pool.rs Send hook metadata to Python worker before running unit tests; PYTHONPATH adjustment
crates/tryke_runner/src/lib.rs Re-export partition_with_hooks
crates/tryke/src/discovery.rs Discovery selection now includes hooks
crates/tryke/src/execution.rs Pass hooks into scheduling (partition_with_hooks) and report cycle execution
crates/tryke/src/main.rs Wire discovered hooks into execution
crates/tryke/src/watch.rs Updated report_cycle signature usage (currently passing empty hooks)
crates/tryke_server/src/handler.rs Server uses partition_with_hooks (currently with empty hooks)
Cargo.lock Version bumps for workspace crates

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…oped hooks

- Enforce single-yield semantics and call gen.close() in generator teardown
- Track async generators with their event loop for proper teardown
- Implement after_all and wrap_all execution via finalize() lifecycle
- Add module_path to HookItem for per-module hook filtering
- Filter hooks to matching modules in schedule and pool (no cross-module broadcast)
- Upgrade DistMode only for modules with constraining hooks, not globally
- Pass hooks in watch mode and server handler (were empty before)
- Fix misleading e2e test comment about after_each
- Add finalize_hooks RPC call after all tests in a work unit complete

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

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

Copilot reviewed 23 out of 24 changed files in this pull request and generated 11 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +359 to +376
# Before_all: run once per scope
for h in sorted(
(h for h in scope_hooks if h.category == "before_all"),
key=lambda h: h.line_number,
):
if (h.category, *h.groups) not in self._all_initialized: # type: ignore[arg-type]
self._resolver.resolve_hook(h.fn, all_scope=True)
self._all_initialized.add((h.category, *h.groups)) # type: ignore[arg-type]

# Wrap_all: resolve on first test in scope (like before_all).
# Generator setup runs now; teardown deferred to finalize().
for h in sorted(
(h for h in scope_hooks if h.category == "wrap_all"),
key=lambda h: h.line_number,
):
if (h.category, *h.groups) not in self._all_initialized: # type: ignore[arg-type]
self._resolver.resolve_hook(h.fn, all_scope=True)
self._all_initialized.add((h.category, *h.groups)) # type: ignore[arg-type]
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

HookExecutor tracks _all initialization using only (category, *groups). If there are multiple @before_all hooks in the same scope, the first one will add e.g. ("before_all", *groups) and subsequent hooks in that scope will be skipped entirely. Track initialization per hook function (e.g., include fn/id(fn) in the key) rather than per (category, groups).

Suggested change
# Before_all: run once per scope
for h in sorted(
(h for h in scope_hooks if h.category == "before_all"),
key=lambda h: h.line_number,
):
if (h.category, *h.groups) not in self._all_initialized: # type: ignore[arg-type]
self._resolver.resolve_hook(h.fn, all_scope=True)
self._all_initialized.add((h.category, *h.groups)) # type: ignore[arg-type]
# Wrap_all: resolve on first test in scope (like before_all).
# Generator setup runs now; teardown deferred to finalize().
for h in sorted(
(h for h in scope_hooks if h.category == "wrap_all"),
key=lambda h: h.line_number,
):
if (h.category, *h.groups) not in self._all_initialized: # type: ignore[arg-type]
self._resolver.resolve_hook(h.fn, all_scope=True)
self._all_initialized.add((h.category, *h.groups)) # type: ignore[arg-type]
# Before_all: run once per scope, per hook function
for h in sorted(
(h for h in scope_hooks if h.category == "before_all"),
key=lambda h: h.line_number,
):
key = (h.category, h.fn, *h.groups) # type: ignore[arg-type]
if key not in self._all_initialized:
self._resolver.resolve_hook(h.fn, all_scope=True)
self._all_initialized.add(key)
# Wrap_all: resolve on first test in scope (like before_all),
# per hook function. Generator setup runs now; teardown deferred
# to finalize().
for h in sorted(
(h for h in scope_hooks if h.category == "wrap_all"),
key=lambda h: h.line_number,
):
key = (h.category, h.fn, *h.groups) # type: ignore[arg-type]
if key not in self._all_initialized:
self._resolver.resolve_hook(h.fn, all_scope=True)
self._all_initialized.add(key)

Copilot uses AI. Check for mistakes.
Comment on lines +416 to +420
teardown_sequence.reverse()
for h in teardown_sequence:
kwargs = self._resolver.resolve(h.fn)
if inspect.iscoroutinefunction(h.fn):
asyncio.run(h.fn(**kwargs))
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

wrap_each teardown (the post-yield half) is executed via DependencyResolver.teardown_generators() after the after_each hooks run. This contradicts the documented order (wrap teardown before after_each) and the intended “wrap around test” semantics. Consider tearing down wrap_each generators before running after_each hooks (or include wrap teardown as part of the teardown sequence).

Copilot uses AI. Check for mistakes.
Comment on lines +90 to +92
let needs_file = hooks
.iter()
.any(|h| h.hook_type.constrains_scheduling() && h.groups.is_empty());
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

needs_file is computed globally across all hooks. If any module has a file-scope _all hook, then all constrained modules are grouped by file, even modules whose _all hooks are only describe-scoped. This unnecessarily reduces parallelism and doesn’t reflect the per-module constraint. Compute file-vs-group needs per module (e.g., a set of file_constrained_modules).

Copilot uses AI. Check for mistakes.
Comment on lines +128 to +135
// Attach hooks to each unit, filtered by the modules in that unit.
if !hooks.is_empty() {
for unit in &mut units {
let unit_modules: std::collections::HashSet<&str> =
unit.tests.iter().map(|t| t.module_path.as_str()).collect();
unit.hooks = hooks
.iter()
.filter(|h| unit_modules.contains(h.module_path.as_str()))
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

Hooks are attached to a WorkUnit solely by module_path (via unit_modules). In --dist group (or when upgrading to group-level units), a unit for one describe group can still carry hooks for other groups in the same module, leading to teardown/setup running on a worker that never executed that scope’s tests. Consider filtering hooks to those relevant to the unit’s tests (e.g., hook.groups being a prefix of some test’s groups).

Suggested change
// Attach hooks to each unit, filtered by the modules in that unit.
if !hooks.is_empty() {
for unit in &mut units {
let unit_modules: std::collections::HashSet<&str> =
unit.tests.iter().map(|t| t.module_path.as_str()).collect();
unit.hooks = hooks
.iter()
.filter(|h| unit_modules.contains(h.module_path.as_str()))
// Attach hooks to each unit, filtered by the modules and groups in that unit.
if !hooks.is_empty() {
for unit in &mut units {
let unit_modules: std::collections::HashSet<&str> =
unit.tests.iter().map(|t| t.module_path.as_str()).collect();
unit.hooks = hooks
.iter()
.filter(|h| {
// First ensure the hook's module matches at least one test in the unit.
if !unit_modules.contains(h.module_path.as_str()) {
return false;
}
// Hooks without groups (module/global scope) apply to all tests in the module.
if h.groups.is_empty() {
return true;
}
// Otherwise, the hook applies only if its groups are a prefix of
// at least one test's groups in this unit.
unit.tests.iter().any(|t| {
if t.groups.len() < h.groups.len() {
return false;
}
h.groups
.iter()
.zip(t.groups.iter())
.all(|(hg, tg)| hg == tg)
})
})

Copilot uses AI. Check for mistakes.
Comment on lines +300 to +304
if let Some(w) = worker.as_mut()
&& let Err(e) = w.finalize_hooks(module).await
{
debug!("worker_task: finalize_hooks failed: {e}");
}
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

finalize_hooks is invoked after every WorkUnit. If a Python module’s tests are split across multiple units (e.g., --dist group, or partition_with_hooks upgrading to group-level units), this will run after_all/wrap_all teardown too early and potentially multiple times. Either ensure partitioning never splits a module when any _all hooks in that module exist, or change finalization to be scoped to the unit/scope rather than the entire module.

Copilot uses AI. Check for mistakes.
Comment on lines +433 to +440
# Build the scope chain from all registered hooks
all_scopes: set[tuple[str, ...]] = set()
for h in self._hooks:
all_scopes.add(tuple(h.groups))
# Process scopes inner-to-outer (longest first for reverse ordering)
sorted_scopes = sorted(all_scopes, key=len, reverse=True)

# Collect after_all hooks across all scopes (inner-to-outer)
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

finalize() builds all_scopes from all registered hooks and then runs every after_all hook it finds, even for scopes that never executed any tests (and therefore never ran their corresponding before_all/wrap_all). This can run teardown without setup for describe blocks with zero tests. Restrict after_all execution to scopes that were actually entered/initialized (e.g., track scopes seen in run_test() and/or consult _all_initialized).

Suggested change
# Build the scope chain from all registered hooks
all_scopes: set[tuple[str, ...]] = set()
for h in self._hooks:
all_scopes.add(tuple(h.groups))
# Process scopes inner-to-outer (longest first for reverse ordering)
sorted_scopes = sorted(all_scopes, key=len, reverse=True)
# Collect after_all hooks across all scopes (inner-to-outer)
# Only consider scopes that were actually initialized during tests.
# This avoids running teardown for describe blocks that never ran.
initialized_scopes: set[tuple[str, ...]] = set(self._all_initialized)
# Process scopes inner-to-outer (longest first for reverse ordering)
sorted_scopes = sorted(initialized_scopes, key=len, reverse=True)
# Collect after_all hooks across all initialized scopes (inner-to-outer)

Copilot uses AI. Check for mistakes.
Comment on lines +95 to +99
let mut units: Vec<WorkUnit> = if mode == DistMode::Test && !constrained_modules.is_empty() {
let (constrained, free): (Vec<_>, Vec<_>) = tests
.into_iter()
.partition(|t| constrained_modules.contains(t.module_path.as_str()));

Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

partition_with_hooks only upgrades scheduling when mode == DistMode::Test. If the user requests --dist group and there are file-scope _all hooks (empty groups), the file can still be split across multiple work units/workers, breaking _all caching semantics. Consider applying the same per-module upgrade logic for DistMode::Group (and ensuring file-scope _all forces file-level grouping regardless of requested mode).

Copilot uses AI. Check for mistakes.
) -> None:
"""Store hook metadata for a module, sent by the runner before tests."""
if not isinstance(hooks, list):
return
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

_register_hooks always invalidates the cached executor for the module. If the same module is executed across multiple work units on the same worker (possible with group-level partitioning), this resets before_all/wrap_all cached state mid-run and can cause repeated setup/incorrect teardown. Either guarantee a module is only registered once per worker-run, or avoid invalidating/rebuilding the executor unless the hook list actually changed (or the module was reloaded).

Suggested change
return
return
# Only update metadata and invalidate the executor if the hooks changed.
prev_hooks = self._hook_metadata.get(module_name)
if prev_hooks is not None and prev_hooks == hooks:
return

Copilot uses AI. Check for mistakes.
Comment on lines +458 to +463
name = h.get("name", "")
groups = list(h.get("groups", []))
line_number = h.get("line_number", 0) or 0
fn = getattr(mod, name, None)
if fn is not None and _hook_category(fn) is not None:
executor.register_hook(fn, groups=groups, line_number=int(line_number))
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

_get_executor assumes every entry in hook_meta is a dict and uses .get() unconditionally. Since _register_hooks only checks hooks is a list, malformed items can raise AttributeError and break test execution. Validate each item is a dict (and that fields like groups have the expected types) before using it.

Suggested change
name = h.get("name", "")
groups = list(h.get("groups", []))
line_number = h.get("line_number", 0) or 0
fn = getattr(mod, name, None)
if fn is not None and _hook_category(fn) is not None:
executor.register_hook(fn, groups=groups, line_number=int(line_number))
# Each hook entry should be a mapping with specific fields; skip malformed items.
if not isinstance(h, dict):
continue
name = h.get("name")
if not isinstance(name, str) or not name:
continue
raw_groups = h.get("groups", [])
if isinstance(raw_groups, (list, tuple)):
groups = list(raw_groups)
else:
groups = []
line_number_val = h.get("line_number", 0)
try:
line_number = int(line_number_val or 0)
except (TypeError, ValueError):
line_number = 0
fn = getattr(mod, name, None)
if fn is not None and _hook_category(fn) is not None:
executor.register_hook(fn, groups=groups, line_number=line_number)

Copilot uses AI. Check for mistakes.
Comment on lines +79 to +80
| Any | `file` or `group` | unchanged | Already grouped — no upgrade needed |

Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

The table implies that if hooks are present and the requested mode is group, the effective mode is “unchanged”. But group allows different groups from the same file to run on different workers, which breaks file-scope _all hooks (they require the whole file on one worker). Either clarify this exception in the docs or ensure scheduling enforces file-level grouping when file-scope _all hooks exist even if --dist group was requested.

Suggested change
| Any | `file` or `group` | unchanged | Already grouped — no upgrade needed |
| `@before_all` at file scope | `group` | `file` | File-scope `_all` hooks require the whole file on one worker |
| `_all` hooks only inside `describe` | `group` | `group` | Groups are already kept together; file can be split across workers |
| Any | `file` | unchanged | Already grouped at file granularity — no upgrade needed |
Note: When you request `--dist group`, any file that defines module-scope
`_all` hooks (such as `@before_all` at file scope) is internally treated
as `--dist file` for that file so that all of its tests run on the same
worker. Other files without such hooks still use group-level distribution.

Copilot uses AI. Check for mistakes.
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