Add xdist support #53
Conversation
Signed-off-by: Travis F. Collins <[email protected]>
Signed-off-by: Travis F. Collins <[email protected]>
Signed-off-by: Travis F. Collins <[email protected]>
Signed-off-by: Travis F. Collins <[email protected]>
Signed-off-by: Travis F. Collins <[email protected]>
Signed-off-by: Travis F. Collins <[email protected]>
Signed-off-by: Travis F. Collins <[email protected]>
Reviewer's Guide by SourceryThis pull request adds support for pytest-xdist, enabling parallel test execution for the pytest-libiio plugin. The changes include modifications to handle custom ports for IIO emulators, adjustments to logging for parallel execution, and the addition of xdist markers for test distribution. File-Level Changes
Sequence DiagramsequenceDiagram
participant Master as Master Process
participant Worker1 as Worker 1
participant Worker2 as Worker 2
participant IIOEmu as IIO Emulator
Master->>Worker1: Start with custom port
Master->>Worker2: Start with custom port
Worker1->>IIOEmu: Initialize (port 30432)
Worker2->>IIOEmu: Initialize (port 30433)
Worker1->>IIOEmu: Run tests
Worker2->>IIOEmu: Run tests
Worker1-->>Master: Report results
Worker2-->>Master: Report results
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @tfcollins - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -14,17 +15,25 @@ | |||
| import pytest_libiio.meta as meta | |||
| import yaml | |||
|
|
|||
| IIO_EMU_BASE_PORT = 30431 | |||
There was a problem hiding this comment.
suggestion (performance): Consider a more dynamic port allocation strategy
While using a base port and incrementing it works for a small number of workers, it might not scale well for larger parallel test runs. Consider implementing a more dynamic port allocation strategy to avoid potential conflicts.
| IIO_EMU_BASE_PORT = 30431 | |
| from contextlib import closing | |
| import socket | |
| def find_free_port(): | |
| with closing(socket.socket(socket.AF_INET, socket.SOCK_STREAM)) as s: | |
| s.bind(('', 0)) | |
| s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | |
| return s.getsockname()[1] | |
| IIO_EMU_BASE_PORT = find_free_port() |
| auto: bool = True, | ||
| rx_dev: str = None, | ||
| tx_dev: str = None, | ||
| custom_port: int = None, |
There was a problem hiding this comment.
suggestion: Add validation for the custom_port parameter
To ensure robustness, consider adding validation for the custom_port parameter. Check if it's within a valid port range (e.g., 1024-65535) before using it.
custom_port: Optional[int] = None,
@staticmethod
def _validate_port(port: Optional[int]) -> None:
if port is not None and not 1024 <= port <= 65535:
raise ValueError("Custom port must be between 1024 and 65535")
def __post_init__(self):
self._validate_port(self.custom_port)
| ) | ||
|
|
||
|
|
||
| def pytest_collection_modifyitems(config, items): |
There was a problem hiding this comment.
suggestion: Add error handling and logging in pytest_collection_modifyitems
Consider adding error handling and logging in this function, especially for cases where the context table is empty or there are issues with markers. This will make debugging easier if something goes wrong during test collection.
def pytest_collection_modifyitems(config, items):
try:
# Get list of contexts and mapped hardware
class Object(object):
pass
context_objs = Object()
except Exception as e:
config.warn(f"pytest_libiio", f"Error in test collection: {str(e)}")
return
| worker_id = worker_id.replace("gw", "") | ||
| custom_port = IIO_EMU_BASE_PORT + int(worker_id) | ||
| logger = logging.getLogger() | ||
| logger.info(f"Using custom port {custom_port}") |
There was a problem hiding this comment.
suggestion: Use debug log level for custom port information
Consider using logger.debug() instead of logger.info() for logging the custom port. This information is more suitable for debug-level logging and will help prevent cluttering the logs during normal operation.
| logger.info(f"Using custom port {custom_port}") | |
| logger.debug(f"Using custom port {custom_port}") |
| @@ -293,14 +338,23 @@ | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope="session", autouse=True) | |||
| def _iio_emu(request): | |||
| def _iio_emu(request, worker_id): | |||
There was a problem hiding this comment.
suggestion: Implement a more generic approach for worker identification
The current implementation is tightly coupled to the specific format of xdist worker IDs. Consider implementing a more generic approach for worker identification that doesn't rely on specific ID formats. This will make the code more flexible and easier to maintain if the worker ID format changes in the future.
def _iio_emu(request, get_worker_id=None):
worker_id = get_worker_id() if get_worker_id else getattr(request.config, 'workerinput', {}).get('workerid', 'master')
| """Tests for verifying plugin works with pytest-xdist.""" | ||
|
|
||
| import pytest | ||
| import iio | ||
| import logging |
There was a problem hiding this comment.
suggestion (testing): Add more comprehensive docstring
Consider expanding the module docstring to provide more context about what these tests are verifying and how they relate to the xdist support feature.
"""
Tests for verifying plugin compatibility with pytest-xdist.
This module contains tests that ensure the plugin functions correctly
when used with pytest-xdist for parallel test execution. It verifies
that the plugin's features work as expected in a distributed testing
environment.
"""
import pytest
import iio
import logging
| for dev in ctx.devices: | ||
| print(f"dev: {dev.name}, {dev.id}") | ||
| logging.info(f"dev: {dev.name}, {dev.id}") |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| for dev in ctx.devices: | ||
| print(f"dev: {dev.name}, {dev.id}") | ||
| logging.info(f"dev: {dev.name}, {dev.id}") |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| for dev in self.data_devices: | ||
| cmd.append(f"{dev}@data.bin") | ||
| if self.custom_port: | ||
| cmd.append("-p") |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace a for append loop with list extend (
for-append-to-extend) - Merge consecutive list appends into a single extend (
merge-list-appends-into-extend)
| """Initialization emulation fixture""" | ||
| if request.config.getoption("--emu"): | ||
| if worker_id == "master": |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Convert for loop into list comprehension (
list-comprehension) - Don't assign to builtin variable
map(avoid-builtin-shadow)
Explanation
Python has a number of builtin variables: functions and constants that
form a part of the language, such as list, getattr, and type
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
list = [1, 2, 3]However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.
Summary by Sourcery
Add support for pytest-xdist to enable parallel test execution by introducing custom port handling and xdist markers. Update the iio_emu_manager class and logging configuration to accommodate these changes. Include pytest-xdist as a dependency and update the iio-emu installation script. Add tests to verify the new functionality.
New Features:
Enhancements:
Build:
Tests: