Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add typing hints to pytest components #1478

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions labgrid/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"""
import os
import warnings
from typing import Dict
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Labgrid sort its imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the way ruff sorts imports with the current configuration in this repo.

Would you prefer them to be in a different order?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I suppose I just don't understand how the ordering works

Perhaps 'warnings' is consider an internal module and 'typing' is not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Ruff puts from imports after imports without the from keyword.

from yaml import YAMLError
import attr

Expand Down Expand Up @@ -262,7 +263,7 @@ def get_imports(self):

return imports

def get_paths(self):
def get_paths(self) -> Dict[str, str]:
"""Helper function that returns the subdict of all paths

Returns:
Expand All @@ -275,7 +276,7 @@ def get_paths(self):

return paths

def get_images(self):
def get_images(self) -> Dict[str, str]:
"""Helper function that returns the subdict of all images

Returns:
Expand Down
6 changes: 3 additions & 3 deletions labgrid/environment.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import os
from typing import Optional
from typing import Callable, Optional
import attr

from .target import Target
Expand All @@ -9,10 +9,10 @@
@attr.s(eq=False)
class Environment:
"""An environment encapsulates targets."""
config_file = attr.ib(
config_file: str = attr.ib(
default="config.yaml", validator=attr.validators.instance_of(str)
)
interact = attr.ib(default=input, repr=False)
interact: Callable[[str], str] = attr.ib(default=input, repr=False)

def __attrs_post_init__(self):
self.targets = {}
Expand Down
2 changes: 1 addition & 1 deletion labgrid/logging.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message is missing the motivation for this change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the message is:

Add this logging level without changing Python’s internal logging module. This maintains architectural integrity and clarifies that the level is labgrid-specific.

I initially found it unclear that the CONSOLE debug level was specific to labgrid. By keeping it separate from Python's internal logging module, it’s immediately evident where this custom level is defined. This distinction is particularly helpful when using tools like pyright in my development environment. The ability to navigate directly to the definition within labgrid's logging module makes this even more clear.

Does this approach make sense to you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I didn't even know that Labgrid had created its own debug level!

Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def basicConfig(**kwargs):
logging.addLevelName(CONSOLE, "CONSOLE")

# Use composition instead of inheritance
class StepFormatter:
class StepFormatter(logging.Formatter):
def __init__(self, *args, indent=True, color=None, parent=None, **kwargs):
self.formatter = parent or logging.Formatter(*args, **kwargs)
self.indent = indent
Expand Down
1 change: 1 addition & 0 deletions labgrid/py.typed
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
partial
10 changes: 10 additions & 0 deletions labgrid/pytestplugin/__init__.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,12 @@
from .fixtures import pytest_addoption, env, target, strategy
from .hooks import pytest_configure, pytest_collection_modifyitems, pytest_cmdline_main

__all__ = [
"pytest_addoption",
"env",
"target",
"strategy",
"pytest_configure",
"pytest_collection_modifyitems",
"pytest_cmdline_main",
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in a separate commit?

21 changes: 14 additions & 7 deletions labgrid/pytestplugin/fixtures.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import os
import subprocess
from typing import Iterator

import pytest

from .. import Environment, Target
from ..exceptions import NoResourceFoundError, NoDriverFoundError
from ..strategy import Strategy
from ..remote.client import UserError
from ..resource.remote import RemotePlace
from ..util.ssh import sshmanager
Expand All @@ -12,7 +16,9 @@
# pylint: disable=redefined-outer-name


def pytest_addoption(parser):
def pytest_addoption(parser: pytest.Parser, pluginmanager: pytest.PytestPluginManager) -> None:
del pluginmanager # unused

group = parser.getgroup('labgrid')
group.addoption(
'--env-config',
Expand Down Expand Up @@ -57,7 +63,7 @@ def pytest_addoption(parser):


@pytest.fixture(scope="session")
def env(request, record_testsuite_property):
def env(request: pytest.FixtureRequest, record_testsuite_property) -> Iterator[Environment]:
"""Return the environment configured in the supplied configuration file.
It contains the targets contained in the configuration file.
"""
Expand All @@ -74,7 +80,8 @@ def env(request, record_testsuite_property):
try:
target = env.get_target(target_name)
except UserError as e:
pytest.exit(e)
pytest.exit(str(e))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems to be unrelated to typing? Or does it fix a bug? If so, best to put the bug-fixes first, in one or more separate commits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this fixes that the wrong type is passed to the pytest.exit() function: pytest.exit.

assert target, "could not get target from environment"
try:
remote_place = target.get_resource(RemotePlace, wait_avail=False)
remote_name = remote_place.name
Expand Down Expand Up @@ -117,7 +124,7 @@ def env(request, record_testsuite_property):


@pytest.fixture(scope="session")
def target(env):
def target(env: Environment) -> Target:
"""Return the default target `main` configured in the supplied
configuration file."""
target = env.get_target()
Expand All @@ -128,13 +135,13 @@ def target(env):


@pytest.fixture(scope="session")
def strategy(request, target):
def strategy(request: pytest.FixtureRequest, target: Target) -> Strategy:
"""Return the Strategy of the default target `main` configured in the
supplied configuration file."""
try:
strategy = target.get_driver("Strategy")
strategy: Strategy = target.get_driver("Strategy")
except NoDriverFoundError as e:
pytest.exit(e)
pytest.exit(str(e))

state = request.config.option.lg_initial_state
if state is not None:
Expand Down
21 changes: 15 additions & 6 deletions labgrid/pytestplugin/hooks.py
Original file line number Diff line number Diff line change
@@ -1,19 +1,22 @@
import os
import warnings
import logging
from typing import List, Optional

import pytest
from _pytest.logging import ColoredLevelFormatter, LoggingPlugin

from .. import Environment
from ..consoleloggingreporter import ConsoleLoggingReporter
from ..util.helper import processwrapper
from ..logging import CONSOLE, StepFormatter, StepLogger

LABGRID_ENV_KEY = pytest.StashKey[Environment]()
LABGRID_ENV_KEY = pytest.StashKey[Optional[Environment]]()


@pytest.hookimpl(tryfirst=True)
def pytest_cmdline_main(config):
def set_cli_log_level(level):
def pytest_cmdline_main(config: pytest.Config):
def set_cli_log_level(level: int):
nonlocal config

try:
Expand All @@ -27,13 +30,15 @@ def set_cli_log_level(level):
current_level = int(logging.getLevelName(current_level))
except ValueError:
current_level = None
assert current_level is None or isinstance(current_level, int), "unexpected type of current log level"

# If no level was set previously (via ini or cli) or current_level is
# less verbose than level, set to new level.
if current_level is None or level < current_level:
config.option.log_cli_level = str(level)

verbosity = config.getoption("verbose")
assert isinstance(verbosity, int), "unexpected verbosity option type"
if verbosity > 3: # enable with -vvvv
set_cli_log_level(logging.DEBUG)
elif verbosity > 2: # enable with -vvv
Expand All @@ -42,7 +47,8 @@ def set_cli_log_level(level):
set_cli_log_level(logging.INFO)


def configure_pytest_logging(config, plugin):
def configure_pytest_logging(config: pytest.Config, plugin: LoggingPlugin):
assert isinstance(plugin.log_cli_handler.formatter, ColoredLevelFormatter), "unexpected type of log_cli_handler.formatter"
plugin.log_cli_handler.formatter.add_color_level(CONSOLE, "blue")
plugin.log_cli_handler.setFormatter(StepFormatter(
color=config.option.lg_colored_steps,
Expand All @@ -61,12 +67,13 @@ def configure_pytest_logging(config, plugin):
plugin.report_handler.setFormatter(StepFormatter(parent=caplog_formatter))

@pytest.hookimpl(trylast=True)
def pytest_configure(config):
def pytest_configure(config: pytest.Config):
StepLogger.start()
config.add_cleanup(StepLogger.stop)

logging_plugin = config.pluginmanager.getplugin('logging-plugin')
if logging_plugin:
assert isinstance(logging_plugin, LoggingPlugin), "unexpected type of logging-plugin"
configure_pytest_logging(config, logging_plugin)

config.addinivalue_line("markers",
Expand Down Expand Up @@ -97,9 +104,11 @@ def pytest_configure(config):
processwrapper.enable_logging()

@pytest.hookimpl()
def pytest_collection_modifyitems(config, items):
def pytest_collection_modifyitems(session: pytest.Session, config: pytest.Config, items: List[pytest.Item]):
"""This function matches function feature flags with those found in the
environment and disables the item if no match is found"""
del session # unused

env = config.stash[LABGRID_ENV_KEY]

if not env:
Expand Down
Empty file added labgrid/pytestplugin/py.typed
Empty file.