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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary

This patch removes the `crypto` test type from the DTS framework and relabels crypto tests as performance tests. The change allows crypto tests to run in any topology, including the new NO_LINK topology mode.

---

## Errors

None found.

---

## Warnings

### 1. Missing check before calling `get_nic_info()`

**File:** `dts/framework/test_run.py`

**Issue:** The code checks if `ctx.topology.sut_ports` exists before calling `get_nic_info()`, but doesn't verify that `self.test_run.ctx.sut_node.main_session` is valid or that the session is connected.

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

**Why it matters:** If the session is not initialized or the connection failed, this could raise an AttributeError or cause unexpected behavior.

**Suggested fix:** Add a session validity check or wrap in try-except if session failures are expected:

```python
testrun_nic_info: list[dict[str, str]] = []
if ctx.topology.sut_ports and self.test_run.ctx.sut_node.main_session:
    testrun_nic_info = self.test_run.ctx.sut_node.main_session.get_nic_info()
```

### 2. Logic change in `topology.py` setup method

**File:** `dts/framework/testbed_model/topology.py`

**Issue:** The devbind script preparation is now called even for NO_LINK topology when cryptodevs are present, but there's no verification that cryptodevs are actually usable or bound before attempting to prepare the script.

```python
if self.type is not LinkTopology.NO_LINK or get_ctx().sut_node.cryptodevs:
    self._prepare_devbind_script()
```

**Why it matters:** If `cryptodevs` is an empty list (falsy but present), the condition behaves differently than if it's None. This could cause the devbind script to be prepared unnecessarily.

**Suggested fix:** Make the cryptodev check more explicit:

```python
has_cryptodevs = get_ctx().sut_node.cryptodevs and len(get_ctx().sut_node.cryptodevs) > 0
if self.type is not LinkTopology.NO_LINK or has_cryptodevs:
    self._prepare_devbind_script()
```

### 3. Documentation needed for empty JSON file behavior

**File:** `dts/framework/test_run.py`

**Issue:** The code writes an empty JSON array to `dut_info.json` when there are no ports, but this behavior isn't documented in the commit message or code comments. Users might be confused by an empty file.

**Suggested fix:** Add a comment explaining this behavior:

```python
# Only collect NIC info if network ports exist.
# An empty JSON array is written as a placeholder if no ports are present.
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()
```

### 4. Condition change from AND to OR may be unintended

**File:** `dts/framework/test_run.py`

**Issue:** The crypto port instantiation condition changed from checking both `cryptodevs` AND `test_run.config.crypto` to checking `cryptodevs` AND `test_run.config.perf`. Since `perf` might be enabled for non-crypto performance tests, this could instantiate crypto ports when not needed.

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

**Why it matters:** Performance tests that don't require crypto devices would now trigger crypto port instantiation if cryptodevs are present.

**Suggested fix:** Consider whether crypto port instantiation should only occur for specific test suites, or if the behavior is intentional for all perf tests. If intentional, add a comment explaining this.

---

## Info

### 1. Code cleanup opportunity

The removal of the `CRYPTO` test type is clean, but there are now only two test types (FUNCTIONAL and PERFORMANCE). Consider whether this enum still provides value or if a boolean flag would suffice in the future.

### 2. Test migration completeness

All test decorators were successfully changed from `@crypto_test` to `@perf_test`. Good consistency across both test files.

### 3. Import statement in method

**File:** `dts/framework/testbed_model/topology.py`

The import of `get_ctx` is inside the `setup()` method:

```python
def setup(self) -> None:
    from framework.context import get_ctx
```

This is likely to avoid a circular import. This is acceptable but could be documented with a comment.

---

## Overall Assessment

The patch is well-structured and makes logical changes to support crypto tests in NO_LINK topology. The main concerns are:

1. Ensuring the session is valid before calling `get_nic_info()`
2. Clarifying the cryptodev check logic
3. Documenting the empty JSON file behavior
4. Verifying the intended behavior of crypto port instantiation with perf tests

The correctness issues are minor and relate to defensive programming rather than outright bugs. The changes to test decorators are mechanical and correct.