lablup / lablup/mlxcel

test(surgery): cover the SurgeryPipeline::apply error path

Open Beginner friendly
#1,693 0 comments 0 reactions 0 assignees View on GitHub
priority:low status:ready type:test
Dominant language
Rust
Stars
467
Forks
54
Avg merge
4h 25m
Merged PRs (30d)
310

Description

## Summary

Add a test for `SurgeryPipeline::apply`'s error path: the error-message shape, that the failing op's name appears, and that ops queued after the failure do not run.

## Background

`apply` wraps an op failure as a formatted string and short-circuits on the first error. The five existing tests cover only the empty and success paths, so nothing pins the error-message format or the short-circuit behavior. The module already has a `NoOp` stub to model a failing op on.

## Proposed Solution

Add a `FailingOp` stub next to `NoOp`, then assert that the returned message contains the op name and the `surgery op \`...\` failed:` prefix, and that an op queued after the failing one records zero invocations. No MLX and no checkpoint needed.

## Implementation Notes

- `src/lib/mlxcel-surgery/src/pipeline.rs:80-86` (confirmed): `apply` maps each op error with `format!("surgery op \`{}\` failed: {e}", op.name())` and propagates with `?`, so a failure short-circuits the loop.
- Existing tests and the `NoOp` stub live at `:118-198`.

## Acceptance Criteria

- [ ] The short-circuit and the message shape are pinned by tests.

---

## Original Suggestion

### Title: test(surgery): cover the SurgeryPipeline::apply error path

`SurgeryPipeline::apply`'s error path has no test: nothing asserts the error-message shape, that the failing op's name appears, or that ops after the failure are not run.

## Evidence

- `src/lib/mlxcel-surgery/src/pipeline.rs:80-86` — failures wrap as `format!("surgery op \`{}\` failed: {e}", op.name())` and short-circuit
- The five existing tests (`:142-198`) cover empty/ok paths only, and the module already has a `NoOp` stub (`:118-198`) to model a `FailingOp` on

## Suggested fix

Add a `FailingOp` stub next to `NoOp` plus assertions: message contains the op name and prefix; an op queued after the failing one records zero invocations. No MLX, no checkpoint.

## Acceptance criteria

- [ ] The short-circuit and the message shape are pinned by tests

Contributor guide

Open the contributing guide

Research direction

Start in src/lib/mlxcel-surgery/src/pipeline.rs, reading SurgeryPipeline::apply at lines 80-86 and the existing tests and NoOp stub at lines 118-198. Add the requested failing-op coverage and run the pipeline tests. Done means the error-message shape and failing operation name are asserted, and an operation queued afterward records zero invocations.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
testing
Issue type
Feature
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.