|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/172648

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-11-06

# DPDK Patch Review

## ERRORS

### 1. Missing Default Assignment Cleanup (Patch 1)

**File:** `dts/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 problematic because when `value == 0`, the first part of the disjunction (`value`) evaluates to `False`, and then the second part (`value == 0`) is checked. This works, but the more Pythonic and clearer way to handle this is to check for `None` explicitly or use `value is not None`. The current form is correct but could lead to confusion if `value` is `False` (a valid boolean value in some contexts).

**Suggested fix:**
```python
value = f"{' ' if is_short else '='}{value}" if value is not None else ""
```

This handles `0`, `False`, empty strings, and other falsy values uniformly while still excluding `None`.

---

### 2. Incorrect Delta Tolerance Logic (Patch 1)

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

```python
# result did not meet the given Gbps parameter, check if within delta.
if getattr(result, "gbps") < expected_gbps:
    if self.delta_tolerance < measured_delta:
        test_result = False
```

The comment says "check if within delta," but the logic says "if delta_tolerance < measured_delta" then fail. This is inverted. If the measured delta is **greater** than the tolerance, the test should fail. The current code has the comparison backwards.

**Correct logic:**
```python
if getattr(result, "gbps") < expected_gbps:
    if measured_delta > self.delta_tolerance:  # measured delta exceeds tolerance
        test_result = False
```

---

### 3. Incorrect Delta Tolerance Logic (Patch 2)

**File:** `dts/tests/TestSuite_cryptodev_latency.py`  
**Lines:** 210-215

```python
# result did not meet the matching parameter, check if within delta.
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:
    if self.delta_tolerance > measured_delta_time:
        test_result = False
```

This is the same inverted comparison as in the throughput test. When `avg_cycles` or `avg_time_us` exceeds the expected value, the test should fail if `measured_delta > delta_tolerance`, not if `delta_tolerance > measured_delta`.

**Correct logic:**
```python
if getattr(result, "avg_cycles") > expected_cycles:
    if measured_delta_cycles > self.delta_tolerance:
        test_result = False
if getattr(result, "avg_time_us") > expected_time:
    if measured_delta_time > self.delta_tolerance:
        test_result = False
```

---

### 4. Division by Zero Risk (Patch 2)

**File:** `dts/tests/TestSuite_cryptodev_latency.py`  
**Lines:** 195-197, 200-202

```python
expected_cycles = parameters.avg_cycles
assert expected_cycles > 0, "Expected average cycles must not be zero"
measured_delta_cycles = abs(
    round((getattr(result, "avg_cycles") - expected_cycles) / expected_cycles, 5)
)
expected_time = parameters.avg_time_us
assert expected_time > 0, "Expected average time must not be zero"
measured_delta_time = abs(
    round((getattr(result, "avg_time_us") - expected_time) / expected_time, 5)
)
```

The assertions protect against division by zero, but assertions can be compiled out in optimized Python (`python -O`). Use runtime checks instead:

**Suggested fix:**
```python
expected_cycles = parameters.avg_cycles
if expected_cycles == 0:
    raise ValueError("Expected average cycles must not be zero")
measured_delta_cycles = abs(
    round((getattr(result, "avg_cycles") - expected_cycles) / expected_cycles, 5)
)
expected_time = parameters.avg_time_us
if expected_time == 0:
    raise ValueError("Expected average time must not be zero")
measured_delta_time = abs(
    round((getattr(result, "avg_time_us") - expected_time) / expected_time, 5)
)
```

---

## WARNINGS

### 1. Undefined Variable Usage (Patch 1, 2)

**Files:** `dts/tests/TestSuite_cryptodev_throughput.py`, `dts/tests/TestSuite_cryptodev_latency.py`  
**Lines:** Multiple occurrences in both files

In nested functions `test()` within each test method, the variable `test_name` is referenced but not defined in the nested function's scope. It should be captured from the enclosing loop:

```python
for combination in self.cipher_tests:
    test_name = combination.name
    combination_results = [test()]  # test() references test_name here
```

While Python's closure mechanism will capture `test_name` from the enclosing scope, this is fragile because `test_name` is reassigned in each loop iteration. If `test()` is called asynchronously or stored for later execution, it could capture the wrong value.

**Suggested fix:**  
Pass `test_name` as a parameter to the nested function, or capture it explicitly in a default argument:

```python
def test(name=test_name) -> list[dict[str, int | float | str]]:
    # use 'name' instead of 'test_name'
```

Or restructure to avoid the nested function pattern entirely.

---

### 2. Inconsistent Optional Parameter Handling (Patch 1)

**File:** `dts/api/cryptodev/__init__.py`  
**Line:** 84-90

```python
def update_params(self, **app_params: Unpack["CryptoPmdParamsDict"]) -> None:
    for k, v in app_params.items():
        if v is not None:
            self._app_params[k] = (
                self.vector_directory.joinpath(str(v)) if k == "test_file" else v
            )
```

The check `if v is not None` skips updating parameters when `v` is `None`, but this means you cannot explicitly set a parameter to `None` to clear it. If this is intentional, it's fine, but if users might want to clear a parameter by passing `None`, this will silently ignore it.

**Suggested fix (if clearing is needed):**  
Either document that `None` values are ignored, or allow `None` to be set explicitly.

---

### 3. Inconsistent Error Logging (Patch 1, 2)

**Files:** `dts/tests/TestSuite_cryptodev_throughput.py`, `dts/tests/TestSuite_cryptodev_latency.py`  
**Multiple locations**

Some exception handlers log with `self._logger.error()` and return an empty list, while others use `self._logger.info()` or no logging at all before returning. For consistency and debugging, all skipped tests should be logged at the same level (preferably `warning` or `info` for skips, `error` for actual errors).

**Example (Patch 1, line ~313):**
```python
except SkippedTestException as e:
    self._logger.error(f"test {test_name} skipped: {str(e)}")
    return []
```

vs. (Patch 1, line ~281):
```python
except SkippedTestException as e:
    self._logger.error(f"failed to run test {test_name}: {str(e)}")
    return []
```

The messages differ: "test skipped" vs. "failed to run test". For a `SkippedTestException`, "skipped" is more accurate.

**Suggested fix:**  
Standardize logging messages and levels for skipped tests vs. actual errors.

---

## INFO

### 1. Unused Import (Patch 1, 2)

**Files:** `dts/tests/TestSuite_cryptodev_throughput.py`, `dts/tests/TestSuite_cryptodev_latency.py`

The import of `LatencyParameters` in the throughput test and `ThroughputParameters` in the latency test are used for type hints in `_create_summary` but could be flagged by linters as unused if the Union type hint is the only usage.

---

### 2. Typo in Docstring (Patch 2)

**File:** `dts/tests/TestSuite_cryptodev_latency.py`  
**Line:** 338

```python
* Run dpdk-test-crypto-perf application with configured parameters in authentication
    only moode.
```

"moode" should be "mode".

---

Review-Result: ERROR