|WARNING| [v2,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/170106

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-28

# DPDK Patch Review

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

### Errors

**In `dts/api/cryptodev/__init__.py` (update_params method):**

1. **Missing cleanup on error path (resource leak):**
   The `update_params` method updates `self._app_params` but does not handle potential exceptions during path manipulation. If an exception occurs after partial updates, the object is left in an inconsistent state.

   ```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
               )
   ```

   **Issue:** If `vector_directory.joinpath()` raises an exception mid-loop, previous `_app_params` entries are already modified but subsequent ones are not, leaving the state corrupted. No rollback mechanism exists.

   **Suggested fix:** Either validate all parameters before modifying state, or use a try-except to restore original state on error:

   ```python
   def update_params(self, **app_params: Unpack["CryptoPmdParamsDict"]) -> None:
       original = self._app_params.copy()
       try:
           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
                   )
       except Exception:
           self._app_params = original
           raise
   ```

**In `dts/tests/TestSuite_cryptodev_throughput.py`:**

2. **RuntimeError raised with no caller handling (unhandled error propagation):**
   In `_create_summary`, when no baseline parameter is found:
   
   ```python
   if parameters == {}:
       raise RuntimeError(errmsg)
   ```

   This exception is not caught in the calling test methods (`cipher_only_tests`, `auth_only_tests`, etc.). The test will abort with an unhandled exception instead of properly failing or logging. The caller catches `SkippedTestException` but not `RuntimeError`.

   **Suggested fix:** Either catch `RuntimeError` in the caller or use `fail()` directly:

   ```python
   if parameters == {}:
       fail(errmsg)
   ```

3. **Incorrect optype in cipher_and_auth_tests:**
   In the `cipher_and_auth_tests` method's `test()` function:

   ```python
   app = Cryptodev(
       ptest=TestType.latency,
       devtype=self.device_type,
       optype=OperationType.aead,  # WRONG
       cipher_algo=CipherAlgorithm[combination["cipher_algorithm"]],
       ...
       auth_algo=AuthenticationAlgorithm[combination["auth_algorithm"]],
       ...
   )
   ```

   **Issue:** `optype=OperationType.aead` is incorrect for cipher-then-auth operations. Should be `OperationType.cipher_then_auth`.

   **Suggested fix:**

   ```python
   optype=OperationType.cipher_then_auth,
   ```

---

### Warnings

**In `dts/framework/params/__init__.py`:**

1. **Fragile zero-value handling:**
   The change from `if value` to `if value or value == 0` is correct for the stated purpose, but the logic is convoluted. Consider:

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

   **Suggested refactor for clarity:**

   ```python
   value = f"{' ' if is_short else '='}{value}" if value is not None else ""
   ```

   This handles zero and all falsy-but-meaningful values uniformly.

**In `dts/tests/TestSuite_cryptodev_throughput.py`:**

2. **Nested function `test()` shadows outer scope:**
   All four test methods define a nested function named `test()`. While legal, this pattern reduces readability. Consider more descriptive names like `run_cipher_test`, `run_auth_test`, `run_aead_test`, `run_cipher_auth_test`.

3. **Empty list returned on skip masks test intent:**
   When a `SkippedTestException` is caught, returning `[]` makes it unclear whether the test was skipped vs. had no buffer sizes configured. The calling code checks `all(result == [] for result in combination_results)` which conflates these cases.

   **Suggested improvement:** Use a sentinel value (e.g., `None`) or a dedicated data structure to distinguish skipped tests from empty results.

4. **Missing release notes:**
   This is a significant refactor changing the public test configuration API for cryptodev throughput tests. Release notes should document:
   - The new `test_combinations` configuration format
   - Migration path from old `throughput_test_parameters`
   - Deprecation of old format

**In `dts/api/cryptodev/config.py`:**

5. **Algorithm parameter dictionaries are module-global constants but named in ALL_CAPS:**
   The naming convention for these (e.g., `CIPHER_ALGORITHM_PARAMS`) suggests they are immutable constants, which they are. This is acceptable, but consider adding docstrings explaining their purpose and structure.

---

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

### Errors

**In `dts/tests/TestSuite_cryptodev_latency.py`:**

1. **Same incorrect optype in cipher_and_auth_tests:**
   Identical bug as in patch 1:

   ```python
   app = Cryptodev(
       ptest=TestType.latency,
       devtype=self.device_type,
       optype=OperationType.aead,  # WRONG
       cipher_algo=CipherAlgorithm[combination["cipher_algorithm"]],
       ...
   )
   ```

   Should be `OperationType.cipher_then_auth`.

2. **Logic error in latency threshold check:**
   In `_create_summary`:

   ```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:
       if self.delta_tolerance > measured_delta_time:
           test_result = False
   ```

   **Issue:** The comparison `self.delta_tolerance > measured_delta_cycles` is backwards. If the measured delta is *smaller* than the tolerance, the test should pass, not fail. The condition should be `measured_delta_cycles > self.delta_tolerance`.

   **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
   ```

3. **RuntimeError raised with no caller handling:**
   Same issue as patch 1 -- `_create_summary` raises `RuntimeError` but callers only catch `SkippedTestException`.

4. **Debug print statement left in code:**
   In `cipher_and_auth_tests`:

   ```python
   for combination in self.cipher_then_auth_tests:
       print(str(combination))  # DEBUG - should be removed
       test_name = combination.get("name", "unnamed_test")
   ```

   Remove the `print()` statement.

---

### Warnings

1. **Duplicate code between throughput and latency test suites:**
   The `set_up_suite`, `_print_stats`, `_print_and_verify`, and test method structure are nearly identical between the two test suites. Consider extracting common functionality to a base class or helper module.

2. **Same nested function naming issue:**
   Functions named `test()` in multiple nested scopes reduce code clarity.

3. **Same empty list on skip pattern:**
   Returning `[]` when `SkippedTestException` is caught conflates skipped tests with empty test configurations.

4. **Missing release notes:**
   Same as patch 1 -- document the new configuration format and migration path.

5. **Assertions on baseline values should be configuration validation:**
   In `_create_summary`:

   ```python
   assert int(expected_cycles) > 0, "Expected average cycles must not be zero"
   assert int(expected_time) > 0, "Expected average time must not be zero"
   ```

   These checks happen during test execution, not during configuration loading. If the configuration is invalid, the suite should fail in `set_up_suite()`, not in the middle of a test. Move these checks to `set_up_suite()` when validating `test_combinations`.

---

## Summary

### Critical Issues (must fix before merge):
- **Incorrect `optype=OperationType.aead` in cipher+auth tests** (both patches) -- will cause wrong test execution
- **Backwards tolerance comparison logic in latency test** (patch 2) -- tests will fail when they should pass
- **Unhandled `RuntimeError` in `_create_summary`** (both patches) -- tests will abort instead of properly failing
- **Debug print statement** (patch 2) -- remove before merge

### Recommended Improvements:
- Add error recovery to `update_params` to avoid leaving object in inconsistent state
- Improve zero-value handling in params module
- Remove code duplication between throughput and latency suites
- Add release notes documenting the configuration format changes
- Move baseline validation to `set_up_suite()` instead of assertions in test execution