|WARNING| [v2] dts: remove crypto test type
dpdklab at iol.unh.edu
dpdklab at iol.unh.edu
Tue Oct 20 01:27:30 CEST 2026
Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/171719
_AI Code Review_
=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-19
# Review of DPDK DTS Crypto Test Type Removal Patch
## ERRORS
None.
## WARNINGS
**1. Empty JSON file written when no ports present** (Warning)
**File:** `dts/framework/test_run.py`, lines 363-368
The patch writes an empty JSON array when `ctx.topology.sut_ports` is empty:
```python
testrun_nic_info: list[dict[str, str]] = []
if ctx.topology.sut_ports:
testrun_nic_info = self.test_run.ctx.sut_node.main_session.get_nic_info()
with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
json.dump(testrun_nic_info, file, indent=3)
```
When no ports are present, this creates a `dut_info.json` file containing `[]`. While this is valid JSON, downstream consumers of this file may need to handle the empty case explicitly. The commit message mentions "an empty json file is written as a placeholder" but doesn't explain if this is intentional or the best approach.
**Suggestion:** Consider either:
- Documenting this behavior in code comments explaining that an empty array is written when no ports exist
- OR, omitting the file entirely when no ports are present (with a log message explaining why)
**2. Conditional topology setup may need verification** (Warning)
**File:** `dts/framework/testbed_model/topology.py`, lines 145-150
The devbind script preparation now runs when either NO_LINK topology with cryptodevs is present, or in normal link topologies:
```python
if self.type is not LinkTopology.NO_LINK or get_ctx().sut_node.cryptodevs:
self._prepare_devbind_script()
if self.type is LinkTopology.NO_LINK:
return
self._setup_ports("sut")
```
This changes the behavior such that `_prepare_devbind_script()` is called even in NO_LINK mode when cryptodevs exist, but `_setup_ports()` is still skipped.
**Suggestion:** Verify that cryptodev tests in NO_LINK mode don't require port setup. The logic appears correct, but the change should be confirmed through testing that cryptodev-only configurations (NO_LINK with cryptodevs) work as expected without calling `_setup_ports()`.
**3. Crypto test gating logic change** (Warning)
**File:** `dts/framework/test_run.py`, line 373
Changed from:
```python
if ctx.sut_node.cryptodevs and test_run.config.crypto:
```
To:
```python
if ctx.sut_node.cryptodevs and test_run.config.perf:
```
This means crypto device setup now runs whenever performance tests are enabled, not just when crypto tests are explicitly enabled. This is a behavior change: if someone runs with `perf=True` but doesn't intend to run crypto tests, they'll still get crypto port instantiation and binding.
**Suggestion:** Consider whether this coupling is intentional. If all crypto tests are now performance tests, this is correct. However, if non-crypto performance tests exist, they may unnecessarily trigger crypto device setup.
## INFO
**Test type removal is consistent**
The removal of `TestCaseType.CRYPTO` and the `crypto_test` decorator is thorough:
- Removed from enum definition
- Removed from decorator export
- Removed from filter logic
- All test cases updated to use `@perf_test` instead
**Configuration field removal**
The `crypto: bool` field is removed from `TestRunConfiguration`, and the filter logic updated to only check `perf` and `func` flags. This is consistent with treating crypto tests as a subset of performance tests.
**Import context in topology.py**
The addition of `from framework.context import get_ctx` inside the `setup()` method is unusual but likely done to avoid circular import issues. This is acceptable though unconventional.
---
Review-Result: WARNING
More information about the test-report
mailing list