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:

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.

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):

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:

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):

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):

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):

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:

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:

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

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:

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

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).

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

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:

# 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.

• 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:

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.