fix typehints in install tests #317
Open
+51
−18
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glehmann
force-pushed
the
gln/install-tests-typehints-srzn
branch
from
0566a19 to
b3e9dea
Compare
ydirson
reviewed
May 28, 2025
tests/install/conftest.py
Outdated
| marker = request.node.get_closest_marker("continuation_of") | ||
| assert marker is not None, "xcpng_chained fixture requires 'continuation_of' marker" | ||
| continuation_of = callable_marker(marker.args[0], request) | ||
| assert isinstance(continuation_of, Sequence) |
There was a problem hiding this comment.
Can't we instead annotate continuation_of (with a possibly more precise type), by annotating callable_marker first?
There was a problem hiding this comment.
I just did as it was already done at the beginning of the file :-)
We could change callable_marker to specify an expected return type:
diff --git a/lib/common.py b/lib/common.py
index 2c8b36e912..1728c51375 100644
--- a/lib/common.py
+++ b/lib/common.py
@@ -67,7 +67,7 @@
logging.debug("scope: %r base: %r relative: %r", scope, base, scoped_nodeid)
return "::".join(itertools.chain(base, (scoped_nodeid,)))
-def callable_marker(value, request):
+def callable_marker(value, request, type):
"""
Process value optionally generated by fixture-dependent callable.
@@ -84,6 +84,7 @@
except pytest.FixtureLookupError as e:
raise RuntimeError("fixture in mapping not found on test") from e
value = value(**params)
+ assert isinstance(value, type)
return value
def wait_for(fn, msg=None, timeout_secs=2 * 60, retry_delay_secs=2, invert=False):
diff --git a/tests/install/conftest.py b/tests/install/conftest.py
index 44dae54907..a6304e7aac 100644
--- a/tests/install/conftest.py
+++ b/tests/install/conftest.py
@@ -329,8 +329,7 @@
# take test name from mark
marker = request.node.get_closest_marker("continuation_of")
assert marker is not None, "xcpng_chained fixture requires 'continuation_of' marker"
- continuation_of = callable_marker(marker.args[0], request)
- assert isinstance(continuation_of, Sequence)
+ continuation_of = callable_marker(marker.args[0], request, Sequence)
vm_defs = [dict(name=vm_spec['vm'],
image_test=vm_spec['image_test'],pyright is clever enough to figure out the type. I have no idea how to properly write the type annotations on callable_marker though
There was a problem hiding this comment.
I had type hints in mind rather than an assert, at first sight there should not be a need for that extra parameter, no?
glehmann
force-pushed
the
gln/install-tests-typehints-srzn
branch
4 times, most recently
from
9c365bb to
e394758
Compare
ydirson
reviewed
Jun 2, 2025
lib/common.py
Outdated
Comment on lines
70
to
94
| def callable_marker(value, request): | ||
| T = TypeVar("T") | ||
|
|
||
| def ensure_type(typ: Type[T], value: Any) -> T: | ||
| """Converts a value to the specified type. Also performs a runtime check.""" | ||
| if not isinstance(value, typ): | ||
| raise TypeError(f"'{type(value).__name__}' object is not of the expected type '{typ.__name__}'") | ||
| return value | ||
|
|
||
| """ |
There was a problem hiding this comment.
Likely a mixup during interactive rebase :)
ydirson
reviewed
Jun 11, 2025
lib/common.py
Outdated
Comment on lines
90
to
92
| if typ in _ensure_type_cache: | ||
| ta = _ensure_type_cache[typ] | ||
| else: | ||
| ta = _ensure_type_cache.setdefault(typ, TypeAdapter(typ)) |
There was a problem hiding this comment.
setdefault already includes the if's logic
glehmann
force-pushed
the
gln/install-tests-typehints-srzn
branch
4 times, most recently
from
1bbef91 to
792f73e
Compare
note that `with_args` is used as a workaround for a pytest type-hint bug that declares the function passed to a marker as returned by the marker, which is not true with a lambda. Signed-off-by: Gaëtan Lehmann <[email protected]>
Signed-off-by: Gaëtan Lehmann <[email protected]>
like TypeDict Signed-off-by: Gaëtan Lehmann <[email protected]>
for a much more efficient type checking of complex types (using pydantic) at runtime: before: 124 μs ± 82.5 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each) after: 1.81 μs ± 4.74 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each) for comparison, when using isinstance: 629 ns ± 9.26 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each) Signed-off-by: Gaëtan Lehmann <[email protected]>
glehmann
force-pushed
the
gln/install-tests-typehints-srzn
branch
2 times, most recently
from
10fa5d6 to
4d9610a
Compare
Signed-off-by: Gaëtan Lehmann <[email protected]> Co-Authored-By: Yann Dirson <[email protected]>
glehmann
force-pushed
the
gln/install-tests-typehints-srzn
branch
from
4d9610a to
36c3085
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.