|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 - cryptodev throughput test suite refactor

## Summary
This patch refactors the cryptodev throughput test suite to support dynamic configuration of algorithm combinations rather than hardcoded test cases. The refactoring is extensive and introduces several correctness issues, primarily around error handling and resource management.

---

## ERRORS

### 1. Resource leak on KeyError in _create_summary
**File:** `dts/tests/TestSuite_cryptodev_throughput.py:164`

The `next()` call with an empty dict as default will cause a KeyError when accessing `parameters["buff_size"]` or `parameters["Gbps"]` if no matching baseline exists. The code raises RuntimeError after this access, but the error message construction itself will fail with KeyError before the RuntimeError is raised.

```python
# Current (line 154-165):
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)
```

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

---

### 2. Wrong operation type in cipher_and_auth_tests
**File:** `dts/tests/TestSuite_cryptodev_throughput.py:444`

The cipher-then-auth test uses `OperationType.aead` when it should use `OperationType.cipher_then_auth`:

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

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

This will cause the test to fail or produce incorrect results as it configures the device for AEAD operations but provides cipher+auth parameters.

---

### 3. Missing digest_size parameter for auth-only tests
**File:** `dts/tests/TestSuite_cryptodev_throughput.py:329`

Auth-only tests set `digest_sz=0` when no digest_size is provided in config, but authentication algorithms require valid digest sizes. The code should use the default from `AUTHENTICATION_ALGORITHM_PARAMS`.

```python
# Line 329 - digest_sz should have a default from auth_params:
digest_sz=combination.get("digest_size", 0),  # WRONG - 0 is invalid
```

**Fix:**
```python
digest_sz=combination.get("digest_size", auth_params["digest_size"]),
```

---

### 4. Uncaught ConfigurationError in set_up_suite
**File:** `dts/tests/TestSuite_cryptodev_throughput.py:130`

The code raises `ConfigurationError` in `set_up_suite` when a test has no algorithm, but this exception is not caught. If this occurs, the test framework may not handle it cleanly, potentially leaving resources allocated or state inconsistent.

The framework should catch this during suite setup and report it as a configuration issue rather than letting it propagate. However, since this is in `set_up_suite`, it may be acceptable if the framework handles all exceptions there. **Verify that ConfigurationError is properly handled by the test framework.** If not, this should be logged and converted to a setup failure.

---

### 5. Use of mutable default for config.test_combinations
**File:** `dts/tests/TestSuite_cryptodev_throughput.py:51`

The `Config` class uses a list literal as the default value for `test_combinations`. This is a mutable default argument anti-pattern in Python. If code modifies this list in place, the modification persists across test runs.

```python
test_combinations: list[dict[str, Any]] = [
    # ... many dict literals ...
]
```

While the current code does not appear to modify the list in place, this is a latent bug. If any future code does `self.config.test_combinations.append(...)`, it would mutate the class default.

**Fix:** Use a factory function or make the default None and initialize in `__post_init__`:
```python
from dataclasses import field

test_combinations: list[dict[str, Any]] = field(default_factory=lambda: [
    {"name": "aes-cbc (cipher only)", ...},
    # ... rest of defaults ...
])
```

---

## WARNINGS

### 1. Swallowed exceptions in test execution loops
**Files:** Multiple test methods

All four test methods (`cipher_only_tests`, `auth_only_tests`, `aead_test`, `cipher_and_auth_tests`) catch `SkippedTestException` inside nested functions, log it, and return an empty list. This silently suppresses the skip for that individual test case while continuing to execute others. The outer loop then checks if all results are empty to skip the entire test method.

This pattern means:
- If one test in a combination is skipped, it's logged but not reported to the test framework
- Only if ALL tests in a method are skipped does the method itself skip
- Partial failures/skips are invisible to the test result summary

**Current pattern (example from line 294-301):**
```python
try:
    return self._create_summary(...)
except SkippedTestException as e:
    self._logger.error(f"test {test_name} skipped: {str(e)}")
    return []
```

**Recommendation:** Consider whether partial skips should propagate up or at least be tracked separately from full test method skips. The current approach may hide device capability issues.

---

### 2. Missing bounds check on buffer_sizes dictionary access
**File:** `dts/tests/TestSuite_cryptodev_throughput.py:122-125`

The code builds `self.buffer_sizes` from `combination["name"]` keys. Later, test methods access `self.buffer_sizes[combination["name"]]` or `self.buffer_sizes[combination.get("name", "custom_test")]`. If a test config has no `"name"` field, the fallback `"custom_test"` will cause a KeyError in `self.buffer_sizes` unless another test also used that name.

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

If `combination` has no `"name"` key and no other test used `"custom_test"`, this will raise KeyError.

**Recommendation:** Either require `"name"` in validation, or build a unique key per test combination.

---

### 3. Inconsistent digest_size handling across operation types
**Files:** Multiple test methods

- Cipher-only: `digest_sz=combination.get("digest_size", 0)` (line 279)
- Auth-only: `digest_sz=combination.get("digest_size", 0)` (line 329) -- should use auth_params default
- AEAD: `digest_sz=combination.get("digest_size", aead_params["digest_size"])` (line 382) -- correct
- Cipher-then-auth: `digest_sz=combination.get("digest_size", 0)` (line 453) -- should use auth_params default

Cipher-only should not need a digest at all (0 is correct). Auth-only and cipher-then-auth need valid digests from algorithm parameters when not specified.

**Already flagged in Error #3 above for auth-only.** Same issue exists in cipher-then-auth.

---

### 4. Hardcoded virtual device names may clash
**File:** `dts/api/cryptodev/config.py:518-563`

The algorithm parameter tables use hardcoded key/IV/digest sizes. Some of these may not be correct for all use cases of those algorithms (e.g., AES-CBC supports 128/192/256-bit keys, but the table hardcodes 16 bytes = 128 bits).

While not strictly a bug (the defaults may be the most common case), this limits flexibility. Consider documenting that these are defaults and can be overridden via test config.

**This is not an error** because the config allows overrides (`cipher_key_size`, etc.), but the comment in the code should note these are defaults, not requirements.

---

### 5. Release notes required
**File:** `doc/guides/rel_notes/` (not present in patch)

This patch significantly changes the test suite configuration format and adds extensive new functionality (dynamic algorithm configuration, parameter tables, test categorization). This affects users who have existing test configurations.

**Recommendation:** Add release notes documenting:
- Changed test configuration format
- Migration guide from old hardcoded test names to new `test_combinations` format
- New algorithm parameter defaults and override capabilities

---

## INFO

### 1. Type annotation inconsistency
**File:** `dts/tests/TestSuite_cryptodev_throughput.py:119`

`self.buffer_sizes` is annotated as `dict[str, ListWrapper]` but accessed with `str` keys throughout. The annotation is correct, but several lines use string literals that are then looked up. Consider extracting test names to constants to catch typos at static analysis time.

---

### 2. Consider extracting test execution logic to a helper
All four test methods (`cipher_only_tests`, `auth_only_tests`, `aead_test`, `cipher_and_auth_tests`) follow the same pattern:
1. Define nested `test()` function
2. Loop over test configurations
3. Catch exceptions and accumulate results
4. Call `_print_and_verify`
5. Handle skip/fail logic

This repetition (150+ lines per method) could be refactored into a parameterized helper that takes:
- The list of test configs
- A callback to build the `Cryptodev` app
- The parameter variations (cipher modes, auth modes)

This would reduce code duplication and make the tests easier to maintain.

---

### 3. Example configuration comment mismatch
**File:** `dts/configurations/tests_config.example.yaml:41-49`

The example config shows field names like `aead_key_sz`, `aead_iv_sz`, `aead_aad_sz`, but the test code uses `aead_key_size`, `aead_iv_size`, `aead_aad_size` (with `_size` suffix, not `_sz`).

**Verify this is intentional.** If the YAML uses different field names than the test code expects, the `combination.get()` calls will always return the defaults.

---

### 4. Documentation URL version reference
**File:** `dts/configurations/nodes.example.yaml:12` and `dts/configurations/tests_config.example.yaml:5`

The patch updates URLs from `guides-24.07` to `guides-26.07`. Since this patch is dated 2026-08-27, version 26.07 would have already been released, so this is likely correct. However, if this is targeting the main development branch, the URL should point to the current development docs, not a past release.

**If targeting DPDK 26.11 or later, consider using `guides/` (no version) or `guides-latest/`.**

---

## VERIFICATION CHECKLIST

- [x] Lines <=100 characters (not applicable to YAML/docs)
- [x] No trailing whitespace (not visible in patch)
- [x] No forbidden tokens
- [x] Proper function format (return type on own line, etc.)
- [x] No deprecated API usage
- [x] Imports properly organized (typing import added at top)
- [ ] Tests added/updated -- **this patch IS a test update**
- [ ] Release notes updated -- **MISSING, see Warning #5**
- [x] No prohibited terminology
- [x] Naming conventions followed

---

## FINAL ASSESSMENT

**This patch has multiple correctness errors that must be fixed before merging:**
1. KeyError risk in `_create_summary` (Error #1)
2. Wrong operation type in cipher_and_auth_tests (Error #2)
3. Invalid digest_size defaults for auth operations (Error #3)
4. Mutable default in Config class (Error #5)

**Additional items requiring attention:**
- Missing release notes (Warning #5)
- Exception handling pattern may hide test failures (Warning #1)
- Missing bounds checks on dictionary access (Warning #2)

The refactoring concept is sound, but the implementation needs these corrections before it can be safely merged.