|WARNING| [v2] dts: remove crypto test type

[dpdklab at iol.unh.edu]

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

# DPDK Patch Review

## ERRORS

None identified.

## WARNINGS

**1. Conditional devbind script preparation introduces fragile ordering dependency**

File: `dts/framework/testbed_model/topology.py`, lines 145-148

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

The logic gates `_prepare_devbind_script()` on either non-NO_LINK topology OR presence of cryptodevs, then immediately returns if topology is NO_LINK. This means cryptodev-only NO_LINK configurations will prepare the script but skip all port setup. While this appears correct for the stated use case (crypto tests in no-link mode), the condition `get_ctx().sut_node.cryptodevs` evaluates at setup time and may be stale if cryptodev configuration changes between setup calls or if the context is reused. The safer pattern would be to pull cryptodev preparation into a separate method or document the assumption that cryptodevs is stable across the setup lifecycle.

Suggested fix: Add a comment explaining the cryptodev check is for NO_LINK + crypto use case, or refactor to make the dependency explicit:

```python
# Prepare devbind script if we have network ports or crypto devices to bind
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:
    # NO_LINK topology has no network ports to set up, but cryptodevs
    # may still require devbind script (already prepared above if present)
    return
```

**2. Empty JSON placeholder without documentation or schema indicator**

File: `dts/framework/test_run.py`, lines 363-368

```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 writes an empty JSON array `[]` to `dut_info.json`. While technically valid JSON, consumers of this file have no way to distinguish "no ports found" from "NIC info collection failed" or "test ran before ports were configured". Consider adding a top-level object with metadata:

```python
if ctx.topology.sut_ports:
    testrun_nic_info = self.test_run.ctx.sut_node.main_session.get_nic_info()
else:
    testrun_nic_info = []

nic_info_output = {
    "timestamp": datetime.now().isoformat(),
    "topology": str(ctx.topology.type),
    "nics": testrun_nic_info,
}
with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
    json.dump(nic_info_output, file, indent=3)
```

This preserves the empty-list behavior for backwards compatibility in the `"nics"` field while providing context.

**3. Cryptodev instantiation gated on config.perf instead of explicit crypto flag**

File: `dts/framework/test_run.py`, line 373

```python
if ctx.sut_node.cryptodevs and test_run.config.perf:
    ctx.topology.instantiate_crypto_ports()
```

The patch removes `config.crypto` and repurposes `config.perf` to gate cryptodev setup. This conflates "performance testing" with "crypto testing". A functional crypto test (if one existed) would now be unable to instantiate crypto ports unless `--perf` is passed. While the commit message states crypto tests are "relabeled as performance tests", this creates a semantic mismatch: not all crypto tests are performance tests, and not all performance tests need cryptodevs.

Suggested fix: Either restore a dedicated `config.crypto` flag for cryptodev instantiation, or rename `config.perf` to something more generic like `config.run_perf_and_crypto` with updated documentation. The current naming implies cryptodevs are only for performance testing.

## INFO

**Configuration schema change not reflected in documentation**

The removal of the `crypto` boolean from `TestRunConfiguration` (line 486) is an API-visible change to the test configuration schema. If this configuration is consumed by external tools or documented in user guides, those references need updating. Verify that `dts/framework/config/` documentation or examples do not reference the removed `crypto` field.

Review-Result: WARNING