|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

## Errors

### 1. Missing error check on algorithm parameter lookup

**File:** `dts/tests/TestSuite_cryptodev_throughput.py`  
**Lines:** 311, 364, 397, 421-426

When looking up algorithm parameters from the dictionaries (`CIPHER_ALGORITHM_PARAMS`, `AUTHENTICATION_ALGORITHM_PARAMS`, `AEAD_ALGORITHM_PARAMS`), the code uses direct dictionary access with `[]` which will raise `KeyError` if the algorithm is not found. This can crash the test instead of providing a clear error message.

```python
# BAD - will raise KeyError if algorithm not in dict
cipher_params = CIPHER_ALGORITHM_PARAMS[
    CipherAlgorithm[combination["cipher_algorithm"]]
]
```

**Suggested fix:** Use `.get()` with a check, or catch `KeyError` and provide a clear error message:

```python
cipher_algo = CipherAlgorithm[combination["cipher_algorithm"]]
cipher_params = CIPHER_ALGORITHM_PARAMS.get(cipher_algo)
if cipher_params is None:
    raise ConfigurationError(f"No parameters defined for cipher algorithm: {cipher_algo.name}")
```

---

### 2. Incorrect operation type in cipher_and_auth_tests

**File:** `dts/tests/TestSuite_cryptodev_throughput.py`  
**Line:** 430

The cipher-then-auth test uses `OperationType.aead` instead of `OperationType.cipher_then_auth`. This is a logic error that will cause incorrect operation.

```python
# BAD - wrong operation type for cipher+auth
app = Cryptodev(
    ptest=TestType.throughput,
    devtype=self.device_type,
    optype=OperationType.aead,  # ERROR: should be cipher_then_auth
    cipher_algo=CipherAlgorithm[combination["cipher_algorithm"]],
    ...
    auth_algo=AuthenticationAlgorithm[combination["auth_algorithm"]],
    ...
)
```

**Suggested fix:**
```python
optype=OperationType.cipher_then_auth,
```

---

## Warnings

### 1. `_print_stats` assumes non-empty test_vals

**File:** `dts/tests/TestSuite_cryptodev_throughput.py`  
**Line:** 175

The function has an `assert` that `test_vals` is non-empty, but `assert` statements are removed when Python runs with `-O`. The calling code in `_print_and_verify` checks `len(results) > 0` before calling, which is good, but the assert-then-use pattern is fragile.

**Suggested fix:** Replace the assert with an early return or proper exception:
```python
if not test_vals:
    return  # or raise ValueError("test_vals must contain at least one element")
```

---

### 2. Empty auth_iv_size overrides algorithm default with 0

**File:** `dts/tests/TestSuite_cryptodev_throughput.py`  
**Lines:** 366, 437

The code uses `.get("auth_iv_size", auth_params["iv_size"])`, which means if a user explicitly sets `auth_iv_size: 0` in the config, the default from `auth_params` is used instead. If 0 is a valid IV size for some algorithms, this is incorrect. (Based on the params fix in `dts/framework/params/__init__.py` line 337, 0 is now treated as a valid value.)

**Current behavior:**
```python
auth_iv_sz=combination.get("auth_iv_size", auth_params["iv_size"])
# If combination["auth_iv_size"] == 0, returns 0 correctly
# But if key doesn't exist, returns default - this is fine
```

Actually, this is correct with `.get()`. No issue here. (Self-correction: omitting this.)

---

### 3. Missing documentation for new Config attributes

**File:** `dts/tests/TestSuite_cryptodev_throughput.py`  
**Lines:** 51-106

The `Config` class docstring describes `delta_tolerance` and `test_combinations`, but the old `throughput_test_parameters` was removed and replaced. The docstring should explicitly note that `ops` is per-test (inherited or overridden), and clarify the structure of `test_combinations`.

**Suggested improvement:** Expand the docstring to include examples or link to the configuration guide.

---

### 4. test_name variable used before assignment in nested functions

**File:** `dts/tests/TestSuite_cryptodev_throughput.py`  
**Lines:** 323, 381, 407, 453

In each test method, `test_name` is read inside the nested `test()` function before it is assigned in the outer loop (`for combination in self.XXX_tests:`). Python resolves this via closure, so `test_name` from the outer scope is visible, but if the loop never runs (empty test list), `test_name` is undefined when the loop body tries to use it in an exception handler.

This is caught by the `test_cases_skipped == len(self.cipher_tests)` check at the end, but the variable is accessed in the `except SkippedTestException` handler inside the loop. If the list is empty, the loop never runs, so `test_name` is never assigned. However, the loop body is only executed if there are tests, so this is not reachable.

**Analysis:** Not an error, but confusing. Consider initializing `test_name = "unknown"` before the loop for clarity.

---

### 5. Inconsistent error messages in exception handlers

**File:** `dts/tests/TestSuite_cryptodev_throughput.py`  
**Lines:** 331, 389, 415, 461

Some handlers say "test {test_name} skipped", others say "failed to run test {test_name}". Both use `self._logger.error()` for a skip, which is misleading. Skips are not errors.

**Suggested fix:** Use `self._logger.warning()` for skips, or `info()`, and keep "error" for actual failures.

---

### 6. Release notes not updated

The patch refactors the test suite significantly (changes configuration structure, removes old test methods, adds new parameterized tests). This is a user-facing change that should be documented in the release notes, but no `doc/guides/rel_notes/` file is modified.

---

### 7. No validation that test names are unique

**File:** `dts/tests/TestSuite_cryptodev_throughput.py`  
**Lines:** 119-146

The code uses `combination["name"]` as a key in `self.buffer_sizes` without checking for duplicates. If two test combinations have the same name, the second will overwrite the first's buffer sizes, causing incorrect test execution.

**Suggested fix:** Check for duplicates in `set_up_suite`:
```python
seen_names = set()
for combination in self.test_combinations:
    name = combination["name"]
    if name in seen_names:
        raise ConfigurationError(f"Duplicate test name: {name}")
    seen_names.add(name)
```

---

## Info

### 1. Line length slightly over 100 in string formatting

**File:** `dts/tests/TestSuite_cryptodev_throughput.py`  
**Lines:** Multiple

Several f-string lines slightly exceed 100 characters (e.g., line 161, 323). This is acceptable per the guidelines (no strict limit), but could be broken up for consistency with the rest of the file.

---

### 2. Use of bare except without re-raise

**File:** `dts/tests/TestSuite_cryptodev_throughput.py`  
**Lines:** 331, 389, 415, 461

The code catches `SkippedTestException` and logs it, then continues. This is correct for the use case (batch execution where one skip shouldn't block others), but consider whether a final summary of skipped tests would be helpful.

---

### 3. Test method docstrings could be more detailed

The new parameterized test methods (`cipher_only_tests`, `auth_only_tests`, etc.) have brief docstrings. Consider adding examples of what algorithms are tested, or referencing the configuration file.

---

## Summary

**Critical issues:**
1. Missing `KeyError` handling on algorithm parameter lookups (will crash on unsupported algorithm)
2. Wrong operation type (`aead` instead of `cipher_then_auth`) in cipher_and_auth_tests

**Important improvements:**
1. Validate unique test names in config
2. Update release notes for user-facing config changes
3. Use appropriate log level for skipped tests (not `.error()`)

The patch is a significant improvement in test configurability and maintainability. The main correctness issue is the operation type bug. After fixing the errors above, it should be ready to merge.