|WARNING| [v3,2/2] dts: refactor cryptodev latency test suite

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-19

# DPDK Patch Review

## Patch 1/2: dts: refactor cryptodev throughput test suite

### ERRORS

**`api/params/__init__.py` line 337:**
```python
value = f"{' ' if is_short else '='}{value}" if value or value == 0 else ""
```
The condition `value or value == 0` is incorrect for checking if a value should be formatted.
When `value` is `0`, Python evaluates `0 or 0 == 0` as `0 or True` which is `True`.
However, when `value` is `None`, this evaluates as `None or False` which is `False`.
The intent appears to be "format if value is not None", but this logic will fail for other falsy values like empty string `""`.
The correct condition should be `value is not None` to explicitly check for None while allowing zero.

**`tests/TestSuite_cryptodev_throughput.py` line 296:**
```python
optype=OperationType.aead,
```
This is incorrect for cipher-then-auth operation.
The function `cipher_and_auth_tests` creates a cipher-then-auth test configuration (it sets both `cipher_algo` and `auth_algo`), but then assigns `optype=OperationType.aead`.
This should be `OperationType.cipher_then_auth`.

### WARNINGS

**`tests/TestSuite_cryptodev_throughput.py` lines 132-136:**
The error message construction pattern:
```python
reason = f"{reason}\n{str(e)}" if reason else str(e)
```
This accumulates multiple verification failures into a single string, which is then passed to `fail()`.
While functional, this loses individual test failure context.
Consider reporting each failure immediately or collecting them in a structured way (list of dicts) rather than concatenating strings.

**`tests/TestSuite_cryptodev_throughput.py` multiple locations:**
The cipher-then-auth test in throughput uses `optype=OperationType.aead` (line 296), but the same test structure in latency correctly uses `optype=OperationType.cipher_then_auth` (see patch 2/2, latency test suite line 294).
This inconsistency is a bug in throughput but correctly fixed in latency.

**Missing release notes:**
This patch adds significant new functionality (complete test configuration coverage for all cryptodev algorithms) but does not update release notes.
The `test_combinations` configuration format is a new user-facing API that should be documented in the release notes.

### INFO

**`dts/api/cryptodev/config.py` lines 518-564:**
The `CIPHER_ALGORITHM_PARAMS`, `AUTHENTICATION_ALGORITHM_PARAMS`, and `AEAD_ALGORITHM_PARAMS` dictionaries provide default key/IV/digest sizes.
These are reasonable defaults, but the source of these values is not documented.
Consider adding a comment indicating where these defaults come from (DPDK test vectors, RFCs, or hardware requirements).

**`tests/TestSuite_cryptodev_throughput.py` line 152:**
```python
errmsg = (
    f"No test parameters found for {test_name} with buffer size {result.buffer_size}"
)
if parameters == {}:
    raise RuntimeError(errmsg)
```
The error message is constructed but `errmsg` is defined on a separate line from the check.
This is fine, but the variable could be inlined into the `raise` statement for brevity.

**Code duplication across test types:**
The four test methods (`cipher_only_tests`, `auth_only_tests`, `aead_test`, `cipher_and_auth_tests`) share nearly identical structure:
- Inner `test()` function that creates `Cryptodev` app
- Exception handling for `SkippedTestException`
- Loop over combinations with skip counting
- Print and verify with accumulated failures

This could be refactored into a helper method that takes the test list and a factory function.
Not required for correctness, but would reduce maintenance burden.

---

## Patch 2/2: dts: refactor cryptodev latency test suite

### ERRORS

**`tests/TestSuite_cryptodev_latency.py` line 294:**
```python
optype=OperationType.aead,
```
Same error as in patch 1: `cipher_and_auth_tests` sets both cipher and auth algorithms, so this should be `OperationType.cipher_then_auth`, not `aead`.

**`tests/TestSuite_cryptodev_latency.py` lines 267-270:**
```python
if getattr(result, "avg_cycles") > expected_cycles:
    if self.delta_tolerance > measured_delta_cycles:
        test_result = False
if getattr(result, "avg_time_us") > expected_time:
```
The comparison logic is inverted.
The test should fail when `measured_delta_cycles` **exceeds** `self.delta_tolerance`, not when tolerance exceeds delta.
Correct condition: `if measured_delta_cycles > self.delta_tolerance`.

Same error on line 270:
```python
if self.delta_tolerance > measured_delta_time:
```
Should be: `if measured_delta_time > self.delta_tolerance`.

This means the test will pass when it should fail (large deviations) and fail when it should pass (small deviations within tolerance).

**`tests/TestSuite_cryptodev_latency.py` line 357:**
```python
print(str(combination))
```
This debug print statement was left in the code and should be removed.

### WARNINGS

**Same issues as patch 1:**
- Missing release notes update
- Error message accumulation pattern (same as throughput)
- Code duplication across test methods

### INFO

**`tests/TestSuite_cryptodev_latency.py` lines 44-46:**
```python
config_list: list[dict[str, int | float | str]] = [
    {"buff_size": 64, "avg_cycles": 999_999_999.0, "avg_time_us": 999_999_999.0},
```
The default baseline values are extremely high (999 million cycles/microseconds).
This effectively disables the test unless explicit baselines are provided.
Consider documenting this intent or providing more realistic default baselines.

**`tests/TestSuite_cryptodev_latency.py` lines 254-259:**
```python
assert int(expected_cycles) > 0, "Expected average cycles must not be zero"
measured_delta_cycles = abs(...)
expected_time = parameters["avg_time_us"]
assert int(expected_time) > 0, "Expected average time must not be zero"
```
These assertions prevent division by zero in delta calculations, which is good.
However, the assertions use `int()` conversion which will truncate float values.
If `expected_cycles` is `0.5`, `int(0.5)` is `0` and the assertion will trigger incorrectly.
Use `assert expected_cycles > 0` directly instead of `assert int(expected_cycles) > 0`.

---

## Summary

Both patches contain the same critical errors:
1. `cipher_and_auth_tests` incorrectly uses `optype=OperationType.aead` instead of `cipher_then_auth`
2. Latency test has inverted comparison logic that will produce wrong pass/fail results

The throughput test comparison logic appears correct (checking if measured value is below expected, then checking delta tolerance), but latency inverts this.

Missing release notes for new user-facing configuration format is a process violation.

Review-Result: ERROR