|WARNING| [v1] dts: refactor cryptodev throughput test suite

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/168478

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-27

# DPDK Patch Review: dts: refactor cryptodev throughput test suite

## Summary

This patch refactors the cryptodev throughput test suite to support configurable test combinations instead of hardcoded test methods. The changes are primarily in the test framework (DTS), not in DPDK core libraries or drivers.

---

## Errors

### 1. Resource leak on error path in `_create_summary`

**Location:** `dts/tests/TestSuite_cryptodev_throughput.py:363`

```python
parameters: dict[str, int | float | str] = next(
    filter(
        lambda x: x["buff_size"] == result.buffer_size,
        params,
    ),
    {},
)
errmsg = (
    f"No test parameters found for {test_name} with buffer size {result.buffer_size}"
)
if parameters == {}:
    raise RuntimeError(errmsg)
```

**Problem:** When `RuntimeError` is raised, the `app` instance created by `Cryptodev()` in the calling functions may hold resources (VFs, virtual devices) that are not cleaned up. The exception propagates up through `test()`, `cipher_only_tests()`, etc., and the `app` object goes out of scope without cleanup.

**Fix:** The calling test methods should wrap `_create_summary` in a try/finally block to ensure cleanup, or `Cryptodev` should implement context manager protocol (`__enter__`/`__exit__`) for automatic cleanup. Alternatively, verify that `Cryptodev` properly cleans up in its destructor or when the process exits.

---

### 2. Wrong `optype` in `cipher_and_auth_tests`

**Location:** `dts/tests/TestSuite_cryptodev_throughput.py:438`

```python
app = Cryptodev(
    ptest=TestType.throughput,
    devtype=self.device_type,
    optype=OperationType.aead,  # BUG: should be cipher_then_auth
    cipher_algo=CipherAlgorithm[combination["cipher_algorithm"]],
    cipher_op=encrypt,
```

**Problem:** The `optype` is set to `OperationType.aead` when it should be `OperationType.cipher_then_auth`. This is a cipher+auth test, not an AEAD test. The function name and docstring confirm it's for "cipher then authentication" mode, but the code uses the wrong operation type.

**Fix:**
```python
optype=OperationType.cipher_then_auth,
```

---

### 3. Logic error: treating empty list as skip indicator

**Location:** Multiple test methods (e.g., lines 301-307, 343-349, 391-397, 464-470)

Pattern:
```python
if all(result == [] for result in combination_results):
    test_cases_skipped += 1
```

**Problem:** An empty list `[]` is used as a sentinel to indicate a skipped test, but this is fragile and ambiguous. It conflates "test was skipped" with "test had no results" (which could be a different error condition). If `_create_summary` or `test()` returns `[]` for any reason other than skip, the logic incorrectly counts it as a skip.

**Fix:** Use an explicit sentinel value (e.g., `None`) or a wrapper object to distinguish skipped tests from other failure modes:
```python
# In test():
except SkippedTestException as e:
    self._logger.error(f"test {test_name} skipped: {str(e)}")
    return None  # or a SkipResult object

# In caller:
if all(result is None for result in combination_results):
    test_cases_skipped += 1
```

---

## Warnings

### 1. Uninitialized field `self.ops` may be incorrect

**Location:** `dts/tests/TestSuite_cryptodev_throughput.py:116`

```python
self.ops: int = 10_000_000
```

**Problem:** The field `self.ops` is initialized in `set_up_suite` to a hardcoded value, but the `Config` class does not have an `ops` field. The code then reads `combination.get("ops", self.ops)` in multiple places, which will always fall back to this hardcoded value if the test config doesn't specify `ops`. This may be intended, but it's not documented and the relationship between the config and the default is unclear.

**Suggestion:** Either add `ops` to the `Config` class with a default of `10_000_000`, or document why the hardcoded value is used as a fallback rather than a config field.

---

### 2. Missing test case for authentication verify mode

**Location:** `dts/tests/TestSuite_cryptodev_throughput.py:343`

```python
combination_results = [(test(AuthenticationOpMode.generate))]
```

**Problem:** Only `AuthenticationOpMode.generate` is tested. The auth API has both `generate` and `verify` modes, but only `generate` is exercised. The cipher tests iterate over all `EncryptDecryptSwitch` modes (encrypt and decrypt), but auth tests only run one mode.

**Suggestion:** Either add `AuthenticationOpMode.verify` to the test matrix, or document why only `generate` is tested.

---

### 3. Inconsistent exception handling between test methods

**Location:** Multiple test methods (cipher_only_tests, auth_only_tests, aead_test, cipher_and_auth_tests)

**Problem:** All four test methods catch `SkippedTestException` in the inner `test()` function and return `[]`, then check if all results are empty and skip. They also catch `TestCaseVerifyError` in `_print_and_verify` and accumulate failure reasons. This pattern is duplicated four times with minor variations. The error handling is inconsistent:
- `cipher_only_tests` logs "test {test_name} skipped"
- `auth_only_tests` logs "failed to run test {test_name}"
- `aead_test` logs "failed to run test {test_name}"
- `cipher_and_auth_tests` logs "failed to run test {test_name}"

**Suggestion:** Extract the common test execution and verification pattern into a helper method to reduce duplication and ensure consistent error handling:
```python
def _run_test_combination(self, test_fn, combinations, test_type_name):
    """Run a list of test combinations and verify results."""
    # common logic here
```

---

### 4. `digest_sz=0` default may be incorrect

**Location:** Multiple places, e.g., line 238

```python
digest_sz=combination.get("digest_size", 0),
```

**Problem:** The default `digest_size` of `0` is used in multiple places when the combination doesn't specify it. For cipher-only tests this may be correct (no digest), but for auth tests a digest size of 0 is likely wrong. The auth algorithm parameter dicts define non-zero digest sizes for all auth algorithms.

**Fix:** Use the algorithm-specific default from the parameter dicts instead of `0`:
```python
# For auth tests:
digest_sz=combination.get("digest_size", auth_params.get("digest_size", 0)),

# For cipher-only tests:
digest_sz=combination.get("digest_size", 0),  # 0 is correct here
```

---

### 5. Type annotation inconsistency

**Location:** `dts/tests/TestSuite_cryptodev_throughput.py:120`

```python
self.buffer_sizes: dict[str, ListWrapper] = {}
```

**Problem:** The type annotation `dict[str, ListWrapper]` is declared, but in `set_up_suite` the dict is populated with entries like:
```python
self.buffer_sizes[combination["name"]] = ListWrapper([...])
```
where `combination["name"]` is `str | Any` (from the test_combinations type). If a test doesn't have a `"name"` key, `.get("name", "custom_test")` is used elsewhere, which is inconsistent.

**Suggestion:** Ensure all test combinations have a `"name"` key (validate in `set_up_suite`), or use a consistent fallback key everywhere.

---

### 6. Missing release notes

**Location:** `doc/guides/rel_notes/`

**Problem:** The patch significantly changes the behavior and configuration of the cryptodev throughput test suite. The old test method names (e.g., `test_aes_cbc`, `test_aesni_mb_vdev`) are removed and replaced with a new config-driven approach. Users upgrading will need to update their test configurations. This is a user-facing change that should be documented in release notes.

**Suggestion:** Add a release note to `doc/guides/rel_notes/release_26_XX.rst` (current release) describing the new configuration format and migration path.

---

## Info

### 1. Consider using Enum for test combination keys

**Location:** Throughout the patch

**Observation:** Test combination dictionaries use string keys like `"name"`, `"cipher_algorithm"`, `"auth_algorithm"`, etc. These are accessed with `.get("key", default)` throughout the code. String keys are error-prone (typos, inconsistent casing) and don't benefit from IDE autocomplete or type checking.

**Suggestion:** Define a TypedDict or dataclass for test combinations:
```python
class TestCombination(TypedDict, total=False):
    name: str
    cipher_algorithm: str
    auth_algorithm: str
    aead_algorithm: str
    cipher_key_size: int
    # ... etc
```
This provides type safety and better documentation of the expected structure.

---

### 2. Magic string "custom_test" as fallback

**Location:** Line 238

```python
buffer_sz=self.buffer_sizes[combination.get("name", "custom_test")],
```

**Observation:** The fallback `"custom_test"` is used only once and is not defined as a constant. If a test combination has no name, this will KeyError unless `"custom_test"` is added to `buffer_sizes`.

**Suggestion:** Either require all combinations to have a `"name"` (validate in `set_up_suite`), or define a constant `DEFAULT_TEST_NAME = "custom_test"` and ensure it's always present in `buffer_sizes`.

---

### 3. Redundant parentheses in list comprehension

**Location:** Lines 296, 339, 460

```python
combination_results = [(test(mode)) for mode in EncryptDecryptSwitch]
```

**Observation:** The parentheses around `test(mode)` are unnecessary. Python style prefers:
```python
combination_results = [test(mode) for mode in EncryptDecryptSwitch]
```

---

### 4. params/__init__.py change is subtle

**Location:** `dts/framework/params/__init__.py:337`

```python
value = f"{' ' if is_short else '='}{value}" if value or value == 0 else ""
```

**Observation:** This changes the boolean check from `if value` to `if value or value == 0`. This allows `value=0` to be passed through, which previously would have been treated as falsy and omitted. This is correct for allowing zero values (e.g., `--digest-size=0`), but the change is unrelated to the cryptodev refactor and could have been a separate commit.

**Suggestion:** Mention this behavioral change in the commit message or split it into a separate preparatory commit.

---

## Positive Observations

1. **Good use of algorithm parameter mappings:** The `CIPHER_ALGORITHM_PARAMS`, `AUTHENTICATION_ALGORITHM_PARAMS`, and `AEAD_ALGORITHM_PARAMS` dicts provide sensible defaults for key/IV/digest sizes, making the configuration more concise.

2. **Config example is comprehensive:** The updated `tests_config.example.yaml` provides clear examples for each test type (cipher only, auth only, cipher then auth, AEAD).

3. **Test categorization is clean:** Splitting test combinations into `cipher_tests`, `auth_tests`, `cipher_then_auth_tests`, and `aead_tests` makes the test structure clear.

4. **`_print_and_verify` reduces duplication:** Extracting the print and verify logic into a helper function is good practice.

---

## Summary of Required Changes

**Must fix (Errors):**
1. Fix `optype=OperationType.aead` to `cipher_then_auth` in `cipher_and_auth_tests`
2. Address resource leak risk on RuntimeError in `_create_summary`
3. Replace empty list `[]` sentinel with explicit skip indicator (`None` or skip object)

**Should fix (Warnings):**
1. Add `digest_sz` defaults from algorithm parameter dicts for auth tests
2. Validate that all test combinations have a `"name"` key in `set_up_suite`
3. Add release notes documenting the configuration change