# Review Test Refactoring

> Review PyTorch test refactoring for correctness and completeness against the decoupling standards defined in the refactor-test-decoupling skill. Accepts a test file path (whole-file review), a PR URL, a git diff, or a branch name. Use this when asked to review a test refactoring PR, check a test decoupling change, verify a refactored test file, review a single test file for classification correctness, or when the user mentions "review" in the context of test splitting, test decoupling, test refactoring, or device-agnostic test migration. Also use when the user opens a PR, diff, or Python test file and asks for a quality check.

- Skill: `karhoutam/review-test-refactoring` (Agent Skill)
- Install (CLI): `npx skillmds@latest add karhoutam/review-test-refactoring`
- Raw SKILL.md: https://api.skillmd.com/api/skills/karhoutam/review-test-refactoring/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: AI & ML
- Author: KarhouTam (https://skillmd.com/u/karhoutam)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/karhoutam/review-test-refactoring

---


# Review Test Refactoring

Review a test file (or a PR/diff) against the decoupling standards defined in
the `refactor-test-decoupling` skill. The review checks classification
correctness, naming conventions, API replacements, instantiation mechanisms,
and completeness.

## How to Use This Skill

### Choose Your Mode

This skill supports two review modes. Determine which applies:

| Input | Mode | What to Review |
|-------|------|----------------|
| A test file path (`test/test_ops.py`) | **Whole-file review** | Audit the entire file against decoupling standards |
| A PR URL, branch name, or `git diff` | **Diff-based review** | Review only the changed portions |

### For Whole-File Review (test file path)

1. **Read the entire test file.** Load the full file — you are auditing every
   class and test method, not just a diff.

2. **Read `../../../reference/device_api_catalog.yaml`.** This catalog is the
   authoritative reference for whether a device API is Category A (has
   `torch.accelerator` equivalent), Category B (general cross-backend concept),
   or Category C (truly device-specific). Every API classification decision in
   the review must be grounded in this catalog. See
   `../../../reference/classification_guide.md` for lookup instructions.

3. Run through the full checklist below, applying every check to every class
   and test method in the file. There is no "before" version to compare against
   — you are the auditor.

### For Diff-Based Review (PR, branch, or diff)

1. Identify the changes to review. If the user provides a PR URL, fetch it with
   `gh`. Otherwise, use `git diff` against the base branch (usually `origin/main`).

2. **Read `../../../reference/device_api_catalog.yaml`** (same as above).

3. For each changed test file, run through the checklist below. Focus on the
   diff — you are reviewing what changed, not re-auditing the entire file.
   However, key checks like naming conventions, instantiation mechanisms, and
   import cleanliness should be verified for the file as a whole even in
   diff-based mode.

### Reporting

Report findings organized by severity:
- **Blocker**: Test loss, wrong classification locking tests out of accelerators,
  broken instantiation (class won't run).
- **Major**: Wrong naming convention, wrong instantiation mechanism,
  stale imports that keep file classified as device_specific.
- **Minor**: Style issues, missed cleanup opportunities, suboptimal
  decorator ordering.

## Reference: The Device API Catalog

`../../../reference/device_api_catalog.yaml` classifies every PyTorch device API into three categories. Always consult it when reviewing classifications — never rely on memory or heuristics.

| Category | Description | Strategy Implication |
|----------|-------------|---------------------|
| **A** | APIs with `torch.accelerator` equivalents | **NOT device-specific** → device-agnostic |
| **B** | General cross-backend concepts, no wrapper yet | **NOT device-specific** → device-agnostic |
| **C** | Truly device-specific, no cross-device equivalent | **device-specific only** |

**Rule**: Only Category C APIs justify device-specific. If a test uses only Category A or B APIs, it must be device-agnostic with `@onlyAccelerator`.

## Review Checklist

### 1. Classification Correctness

The single most impactful category of review finding. A wrong classification
either locks tests out of accelerators they could run on (device-agnostic
misclassified as device-specific) or causes test failures on accelerators that lack
the required features (device-specific misclassified as device-agnostic).

#### 1a. False-CUDA Detection (most common error)

For every test classified as device-specific (`TestFooCUDA`) or using
`@onlyCUDA` / `device="cuda"`, ask:

> Is this test verifying a truly device-specific feature (Category C in the
> report), or is it just using CUDA as a device for generic computation?

**How to check**: Look up each `torch.cuda.*` API the test uses in
`../../../reference/device_api_catalog.yaml`. If every API it uses is Category A or B,
the test is misclassified — it should be device-agnostic with `@onlyAccelerator`.

**Red flags** (signals the test is wrongly classified as CUDA-specific):

| Code Pattern | What It Means | Severity |
|-------------|---------------|----------|
| Test with generic ops (add, softmax, matmul, loss) still has `@onlyCUDA` or `device="cuda"` | Should be device-agnostic with `@onlyAccelerator` | Blocker |
| `.cuda()` / `.to("cuda")` used instead of `.to(device)` | Test hardcodes CUDA for no reason | Blocker |
| `torch.cuda.<api>` call where the catalog shows `torch.accelerator.<api>` exists | Category A — has cross-accelerator equivalent; replace with `torch.accelerator.*` | Major |
| `torch.cuda.Stream` / `torch.cuda.Event` used but test not marked as device-specific | Category B — general concept; verify usage context, usually device-agnostic | Info |
| `TEST_CUDA` import remains but no device-specific CUDA tests exist in the file | Stale import keeps file classified as device_specific | Major |

**Unnecessary `@onlyAccelerator`**: If `@onlyAccelerator` was ADDED to a test that had no prior device restriction, verify the test genuinely requires an accelerator. If it works on CPU, the restriction should have been removed entirely.

#### 1b. Over-generalization Detection

Conversely, check that tests using Category C APIs were NOT incorrectly
generalized to device-agnostic. Consult `../../../reference/device_api_catalog.yaml` → `category_c` for the full per-backend lists. Key examples:

| Code Pattern | What It Means | Severity |
|-------------|---------------|----------|
| Test using any API from `category_c.cuda` in the catalog but placed in `TestFooDevice` with `@onlyAccelerator` | Will fail on non-CUDA accelerators | Blocker |
| Test using any API from `category_c.mps` in the catalog but placed in `TestFooDevice` | Will fail on non-MPS accelerators | Blocker |
| Test using any API from `category_c.xpu` in the catalog but placed in `TestFooDevice` | Will fail on non-XPU accelerators | Blocker |

**Dtype compatibility on MPS**: Even when a test uses only generic ops (no
Category C APIs), it may still fail on non-CUDA accelerators if it uses dtypes
not supported by that backend. The most common case: `complex128` and `float64`
are unsupported on MPS. When a test is generalized to device-agnostic (or already
uses `@onlyAccelerator`):

| Check | How to Verify |
|-------|---------------|
| `complex128` or `torch.complex128` used in `@dtypes` or as default dtype | MPS does not support double-precision complex. Add `@expectedFailureMPS` or use `@dtypesIfMPS` to exclude `complex128`. |
| `float64` or `torch.float64` used in `@dtypes` or as default dtype | MPS does not support float64. Add `@expectedFailureMPS` or use `@dtypesIfMPS` to exclude `float64`. |
| `torch.long` used with MPS convolution/indexing ops | MPS has limited int64 support in some ops. Add a skip (`@skipIfMPS`) if needed. |

**How to validate**: For every test using `@onlyAccelerator` or the `device`
parameter, verify every dtype it exercises (from `@dtypes`, `_default_dtype`, or
inline tensor creation) against known MPS dtype limitations. The failure
signature is: `"Cannot convert a MPS Tensor to float64 dtype"` or similar dtype
conversion errors on MPS. In diff-based mode, pay special attention to tests
where `@onlyCUDA` was removed.

**MPS coverage safety**: When MPS coverage is broadened (new `allow_mps=True` or `@onlyAccelerator` replacing CUDA-only restriction), verify `@skipIfMPS` is present unless MPS was already covered via `@dtypesIfMPS` or `@onlyMPS`. Exception: `@skipIfMPS` is NOT required when the class is NOT instantiated for MPS — MPS variants are only created when the class's `instantiate_device_type_tests` call passes `allow_mps=True`. If no MPS variant exists, the test cannot run on MPS and the skip is unnecessary.

**@onlyCPU to device-agnostic**: Verify each `@onlyCPU` test was individually evaluated (not bulk-decided). Check that `device` param was added when `@onlyCPU` was removed.

#### 1c. CPU-only Correctness

For tests in a CPU-only class (`TestFoo` without device suffix):

| Check | What to Look For |
|-------|-----------------|
| No `device` parameter in method signature | `def test_foo(self)` not `def test_foo(self, device)` |
| No device-dependent decorators | No `@onlyCUDA`, `@onlyAccelerator`, `@skipCUDAIf`, etc. |
| No `.to(device)`, `.cuda()`, `device=device` in test body | All tensors are CPU |
| No cross-device tensor operations | Can't have CPU tensor op GPU tensor |

### 2. Naming Convention

Class renaming is **OPTIONAL**. The future `hw_classification` member on TestCase will handle strategy classification; class names are no longer the primary discriminator. The coder decides whether to rename based on external reference impact (see `refactor-test-decoupling` for the decision framework).

**Do NOT flag a class name mismatch as an issue unless it is actively misleading** (e.g., a CPU-only class named `TestFooCUDA`). A class using the original name with the correct strategy mechanism is valid.

For reference, the recommended naming convention (when coder chooses to rename):

| Strategy | Recommended Name | Acceptable Alternative |
|----------|-----------------|----------------------|
| CPU-only | `TestFoo` (original name) | Must NOT have device suffix |
| device-agnostic | `TestFooDevice` | Original name is fine |
| device-specific | Original name (`instantiate_device_type_tests` appends the device suffix) | Original name is fine |

#### 2a. Cross-File Reference Integrity

**If the coder kept the original class names, skip this check** — no external references need updating. This is the primary benefit of not renaming.

**When a class IS renamed** (e.g., `TestIndexing` → `TestIndexingDevice`), the
old name may still be referenced in external configuration files. A rename
without updating these files causes CI breakage: dynamo expected-failure entries
stop matching, turning expected failures into unexpected failures.

**Files to check for stale class name references:**

| File/Directory | Example Reference | How to Verify |
|---------------|-------------------|---------------|
| `test/dynamo_skips/` | `TestIndexing.test_invalid_sparse_coo_values_cpu` | `find test/dynamo_skips/ -name "OldClassName*"` **(by filename, NOT grep — these are sentinel files, often 0 bytes)** |
| `test/dynamo_expected_failures/` | `TestIndexingCPU.test_byte_mask_cpu` | `find test/dynamo_expected_failures/ -name "OldClassName*"` **(by filename, NOT grep)** |
| `test/inductor_expected_failures/` | `TestIndexing.test_foo` | `find test/inductor_expected_failures/ -name "OldClassName*"` **(by filename, NOT grep)** |
| `torch/testing/_internal/common_methods_invocations.py` | `DecorateInfo(unittest.skip("..."), 'TestCommon', 'test_complex_half_reference_testing')` | Search for `'OldClassName'` string in `DecorateInfo(...)` constructor calls |
| `.ci/pytorch/test_exclude_list.py` | Test name in skip list | `grep -r "OldClassName\b" .ci/pytorch/` |
| `.ci/pytorch/*-trunk.yml` | Test name in CI config | `grep -r "OldClassName\b" .ci/` |

**How to fix:** For each stale reference, update the class name to match the
new name. Verify which class actually owns each test — when a class is split
into multiple new classes (CPU-only + 2 + 3), tests may now live under
different class names (e.g., `TestIndexing.test_foo` might now be
`TestIndexingDevice.test_foo` or `TestIndexingCPU.test_foo`).

**For `common_methods_invocations.py` specifically:** `DecorateInfo` entries
use exact class name matching in `is_active()` — if `cls_name='TestCommon'`
but the test now lives in `TestCommonDevice`, the skip/xfail decorator is
silently dropped. To find broken entries:

```bash
python -c "
import torch
from torch.testing._internal.common_methods_invocations import op_db
from torch.testing._internal.opinfo.core import DecorateInfo

old_names = {'TestOldName1', 'TestOldName2'}  # fill in renamed classes
count = 0
for op in op_db:
    for d in op.decorators:
        if isinstance(d, DecorateInfo) and d.cls_name in old_names:
            print(f'{op.name}: cls_name={d.cls_name}, test_name={d.test_name}')
            count += 1
print(f'Total: {count} stale DecorateInfo entries')
"
```

Then replace the old class name string literal with the new one in
`torch/testing/_internal/common_methods_invocations.py`.

**Severity**: Blocker — broken DecorateInfo entries cause tests to silently
run when they should be skipped, or tests to be silently skipped when they
should run.

### 3. Instantiation Mechanism

| Strategy | Expected Mechanism | Wrong Mechanism |
|----------|-------------------|-----------------|
| CPU-only, no parametrization | Plain `TestCase` | `instantiate_device_type_tests` |
| CPU-only, with `@parametrize`/`@dtypes` | `@instantiate_parametrized_tests` | `instantiate_device_type_tests` |
| CPU-only, with `@ops` | `instantiate_device_type_tests(..., only_for="cpu")` | `@instantiate_parametrized_tests` |
| device-agnostic | `instantiate_device_type_tests(TestFooDevice, globals())` | `@instantiate_parametrized_tests` |
| device-specific | `instantiate_device_type_tests(..., only_for="<device>")` | Plain `TestCase` with `setUp` guard or `@instantiate_parametrized_tests` |

**Critical**: Check that `instantiate_device_type_tests` is never used for
CPU-only classes — it creates useless per-device variants.

**Critical**: Check that no class uses both `instantiate_parametrized_tests`
and `instantiate_device_type_tests` — double instantiation causes test name
collisions.

### 4. API Replacement Correctness

For device-agnostic tests, verify device-specific APIs were replaced with their
device-agnostic equivalents. **Consult `../../../reference/device_api_catalog.yaml` → `category_a` for the authoritative mapping.** The catalog defines every `torch.<device>.<api>` → `torch.accelerator.<api>` replacement.

**Key checks:**

| Before (Wrong) | After (Correct) | Check |
|---------------|-----------------|-------|
| `@onlyCUDA` | `@onlyAccelerator` | Not left as `@onlyCUDA` |
| `@unittest.skipIf(not TEST_CUDA, ...)` | `@onlyAccelerator` | Not left as skip |
| `device="cuda"` | `device` parameter | No hardcoded `"cuda"` |
| `.cuda()` / `.to("cuda")` | `.to(device)` | No `.cuda()` calls |

For any `torch.cuda.<api>()` call remaining in a device-agnostic test, check the
catalog: if Category A, it should be `torch.accelerator.<api>()`. If Category B,
use the unified type (e.g., `torch.Stream` instead of `torch.cuda.Stream`). If
Category C, the test belongs in device-specific.

**Return type compatibility**: Verify return type compatibility for all `torch.accelerator.*` replacements, especially HIGH RISK APIs: `current_device_index` (returns `int`, compare against `int`), `set_device_index` (takes `int` arg), `get_device_capability` (return type differs across backends). Consult `../../../reference/device_api_catalog.yaml` type annotations.

**Remaining `torch.cuda` in device-agnostic classes**: Scan each device-agnostic class for remaining `torch.cuda.*` calls — each must be either migrated to `torch.accelerator.*` or the test moved to device-specific.

### 5. Import Cleanup

| Check | How to Verify |
|-------|---------------|
| `TEST_CUDA` import removed if no device-specific CUDA tests remain | `grep "TEST_CUDA"` in the file |
| `TEST_MPS` import removed if no device-specific MPS tests remain | `grep "TEST_MPS"` in the file |
| `TEST_XPU` import removed if no device-specific XPU tests remain | `grep "TEST_XPU"` in the file |
| `@onlyCUDA` import removed if no device-specific CUDA tests remain | `grep "onlyCUDA"` in the imports |
| `@onlyOn` import removed if all uses were replaced | `grep "onlyOn"` in the file |
| `@onlyNativeDeviceTypes` removal | `@onlyNativeDeviceTypes` / `@onlyNativeDeviceTypesAnd` are redundant on device-agnostic classes — REMOVE them. Before removing, verify dtype compatibility (`float64`/`complex128`/channels-last may be unsupported on MPS/MTIA) and add `@skipIfMPS`/`@dtypesIfMPS` if needed. |
| New imports are correct | `onlyAccelerator` from `common_device_type`, `torch.accelerator` if used |

### 6. Test Completeness

**Whole-file review**: Verify every test method in the file is properly placed in
an appropriate strategy class. No test should be in a class that doesn't match
its device dependency level (e.g., a test using only CPU ops should not be in a
`TestFooCUDA` class).

| Check | How to Verify |
|-------|---------------|
| Every `def test_` belongs to the correct strategy class | Cross-reference each test's API usage against the catalog and its enclosing class name |
| Device instantiation present for device-specific | `instantiate_device_type_tests(..., only_for="<device>")` with a `device` param on each method |
| No test logic unintentionally modified | If reviewing a diff, compare test bodies against the base version. If whole-file, flag tests that appear incomplete or have empty bodies |
| No duplicate test bodies across device-specific classes | If identical test bodies appear across device-specific classes, they belong in the device-agnostic shared class |
| No device-specific artifacts in device-agnostic classes | Scan for `_cuda` suffix in test method names, internal variable names like `cuda_out`, module-level helpers with `if device_type == "<backend>"` branches — clean these when the test is in a device-agnostic class |

**Diff-based review**: Additionally verify that every original test method is
accounted for (count `def test_` in old vs new). A test "lost" in refactoring is
a regression.

### 7. Common Pitfalls

| Pitfall | Detection | Severity |
|---------|-----------|----------|
| `@onlyAccelerator` used as class decorator | `@onlyAccelerator\nclass TestFoo` — breaks `instantiate_device_type_tests` | Blocker |
| CPU-only class has `device` parameter | `def test_foo(self, device)` in `TestFoo` (no Device suffix) | Blocker |
| `skipIfXpu`/`skipIfCUDA` from `common_utils` in device-agnostic class | These skip ALL variants, not just the target device | Major |
| `GPU_TYPE`/`HAS_GPU` from `inductor_utils` not converted | Leftover inductor-specific device abstraction | Major |
| Mixed `device` param and hardcoded `"cuda"` in same class | Inconsistent; some tests use device param, others hardcode | Major |
| `instantiate_device_type_tests` call references wrong class name | Class name in `globals()` call doesn't match actual test class — class never instantiated | Blocker |
| `except_for`/`only_for`/`allow_mps`/`allow_xpu` args missing from instantiation | Device allowlists not applied to current `instantiate_device_type_tests` call | Major |
| Category A/B API treated as if it makes a test CUDA-specific | Test locked to CUDA unnecessarily; check the catalog | Major |
| Missing blacklist skip decorators | `@skipXPU`, `@skipMPS`, `@skipMeta` absent — these document known gaps. If the original file had them and they're now gone, that's a regression | Blocker |
| `@onlyAccelerator` used without dtype compatibility check | Test runs on MPS/XPU but uses `complex128` or `float64` (unsupported on MPS). For every test using `@onlyAccelerator`, verify every dtype the test uses is supported on ALL target backends. If not, add `@expectedFailureMPS`, `@dtypesIfMPS`, or a skip decorator. | Blocker |
| Test class name doesn't match OpInfo DecorateInfo references | `DecorateInfo` entries in `common_methods_invocations.py` use exact class name matching in `is_active()`. If the test class was RENAMED, verify DecorateInfo entries were updated (section 2a). If the coder kept the original name, this check is a no-op. | Blocker |
| **Flagging an original class name as "wrong" when the coder chose not to rename** | Renaming is optional. If the coder kept the original name (e.g., `TestFoo` for a device-agnostic class), do NOT flag it unless the name is actively misleading (e.g., a CPU-only class named `TestFooCUDA`). The `hw_classification` member will handle classification. | N/A — reviewer guidance |
| `@unittest.skipIf(not TEST_CUDA, ...)` leftover in device-agnostic class | Should be `@onlyAccelerator` | Major |
| `@skipIfMPS`/`@skipXPU`/`@skipCUDAIf` applied to method without `device` parameter | These decorators check the `device` kwarg and silently fail if missing | Blocker |
| Missing `hw_classification` class attribute | Every test class must have `hw_classification = HardwareClassification.XXX`. Missing attr causes test runner to skip or misroute tests. | Blocker |
| Incorrect `hw_classification` value | Value must match the class mechanism: GENERIC for CPU-only, ACCELERATOR for device-agnostic, CUDA/MPS/XPU for device-specific per device, CPU for CPU-only-with-`@ops`. A wrong value (e.g., GENERIC on a device-agnostic class) breaks `--hw-classification` filtering. | Blocker |
| `HardwareClassification` not imported | Must be imported from `torch.testing._internal.common_utils` and merged alphabetically into the existing import block. | Blocker |

### 8. Decorator Ordering

For device-agnostic tests, decorators must be ordered correctly. The `device`
parameter is filled in by `instantiate_device_type_tests`, and other
parametrization decorators fill additional arguments:

```python
# Correct: @dtypes closest to method, @onlyAccelerator above
@onlyAccelerator         # outermost (skip if CPU)
@dtypes(torch.float32)   # parametrization
def test_foo(self, device, dtype):
    ...

# Wrong — @onlyAccelerator below @dtypes may cause issues
@dtypes(torch.float32)
@onlyAccelerator
def test_foo(self, device, dtype):  # incorrect
    ...
```

### 9. HardwareClassification Tag

Every test class must have a `hw_classification` class attribute matching its
strategy and instantiation mechanism. **This is mandatory** — the test runner
uses `--hw-classification` to filter test execution by hardware category.

| Class Mechanism | Expected `hw_classification` |
|----------------|------------------------------|
| Plain `TestCase` or `@instantiate_parametrized_tests`, no `device` param | `HardwareClassification.GENERIC` |
| `instantiate_device_type_tests(only_for="cpu")` | `HardwareClassification.CPU` |
| `instantiate_device_type_tests(except_for=...)` | `HardwareClassification.ACCELERATOR` |
| `instantiate_device_type_tests(only_for="cuda")` | `HardwareClassification.CUDA` |
| `instantiate_device_type_tests(only_for="mps")` | `HardwareClassification.MPS` |
| `instantiate_device_type_tests(only_for="xpu")` | `HardwareClassification.XPU` |
**Structural contract (mirrors the deterministic test linter):**
- `GENERIC`: class NOT instantiated via `instantiate_device_type_tests`; methods take no `device`/`devices`.
- `ACCELERATOR`: class instantiated via `instantiate_device_type_tests`; every method takes `device`/`devices`; no `@only*` except `@onlyAccelerator`; the instantiate call uses no `only_for`.
- `CPU`/`CUDA`/`MPS`/`XPU`: class instantiated via `instantiate_device_type_tests` with `only_for=<device>`; every method takes `device`/`devices`; the instantiate call uses no `except_for`.

**How to verify:**

| Check | How to Verify |
|-------|---------------|
| Import present | `grep "HardwareClassification" <file>` — must be imported from `torch.testing._internal.common_utils` |
| Every class tagged | `grep "hw_classification" <file>` — count must equal number of TestCase subclasses |
| Value matches strategy | Cross-reference each class's mechanism (instantiate call + device params) against the table above |
| Import merged alphabetically | `HardwareClassification` must appear in the existing `common_utils` import block in alphabetical order |

**Severity**: Blocker — missing or incorrect `hw_classification` causes the test runner to skip or misroute tests.

## Review Output Format

Structure your review as follows:

```
## Review: <test file path or PR/branch name>

### Summary
- Mode: whole-file / diff-based
- File(s) reviewed: N
- Classification: correct / N issues found
- Naming: correct / N issues found
- API replacements: correct / N issues found
- Completeness: all tests properly placed / N issues found

### Findings

#### Blockers (must fix)
- [ ] **<file:line>**: <issue description>
  - Fix: <suggested fix>

#### Major (should fix)
- [ ] **<file:line>**: <issue description>

#### Minor (nice to have)
- [ ] **<file:line>**: <issue description>

### Verified Correct
- <list of things that are correct per the standards>
```

## Reference

The refactoring standards this review checks against are defined in the
`refactor-test-decoupling` skill. Consult it for the full classification
decision tree, blacklist vs. whitelist rules, instantiation mechanism
comparison, and CPU-only/2/3 patterns.

**`../../../reference/device_api_catalog.yaml`** is the single authoritative source for API classification. It categorizes every device API as A (accelerator equivalent), B (general concept), or C (truly device-specific). All classification decisions in the review must be grounded in this catalog — never hard-code or guess which APIs belong to which category.