This repository was archived by the owner on Jan 23, 2026. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 21
Gracefully handle missing driver client imports #805
Draft
evakhoni
wants to merge
5
commits into
jumpstarter-dev:main
Choose a base branch
from
evakhoni:handle_missing
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
b83340c
gracefully handle missing drivers with stub client
evakhoni 1405cd7
tests for missing driver and stub
evakhoni 6052f8b
handle missing driver at exporter side separately
evakhoni 6a174f3
Merge remote-tracking branch 'upstream' into handle_missing
evakhoni 82ee10e
suppress double warning, handle missing class
evakhoni File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| """Tests for StubDriverClient.""" | ||
|
|
||
| import logging | ||
| from contextlib import ExitStack | ||
| from unittest.mock import MagicMock, create_autospec | ||
| from uuid import uuid4 | ||
|
|
||
| import pytest | ||
| from anyio.from_thread import BlockingPortal | ||
|
|
||
| from .base import StubDriverClient | ||
| from jumpstarter.common.utils import serve | ||
| from jumpstarter.driver import Driver | ||
|
|
||
|
|
||
| class MissingClientDriver(Driver): | ||
| """Test driver that returns a non-existent client class path.""" | ||
|
|
||
| @classmethod | ||
| def client(cls) -> str: | ||
| return "nonexistent_driver_package.client.NonExistentClient" | ||
|
|
||
|
|
||
| def create_stub_client(class_path: str) -> StubDriverClient: | ||
| """Create a StubDriverClient with minimal mocking for testing.""" | ||
| return StubDriverClient( | ||
| uuid=uuid4(), | ||
| labels={"jumpstarter.dev/client": class_path}, | ||
| stub=MagicMock(), | ||
| portal=create_autospec(BlockingPortal, instance=True), | ||
| stack=ExitStack(), | ||
| ) | ||
|
|
||
|
|
||
| def test_missing_driver_logs_warning_and_creates_stub(caplog): | ||
| """Test that a missing driver logs a warning and creates a StubDriverClient.""" | ||
| expected_class_path = "nonexistent_driver_package.client.NonExistentClient" | ||
| with caplog.at_level(logging.WARNING): | ||
| with serve(MissingClientDriver()) as client: | ||
| # Should have logged a warning with the exact class path from MissingDriverError | ||
| assert f"Driver client '{expected_class_path}' is not available." in caplog.text | ||
|
|
||
| # Should have created a StubDriverClient | ||
| assert isinstance(client, StubDriverClient) | ||
|
|
||
| # Using the stub should raise an error | ||
| with pytest.raises(ImportError): | ||
| client.call("some_method") | ||
|
|
||
|
|
||
| def test_stub_driver_client_streamingcall_raises(): | ||
| """Test that streamingcall() raises ImportError with driver info.""" | ||
| stub = create_stub_client("missing_driver.client.Client") | ||
| with pytest.raises(ImportError) as exc_info: | ||
| # Need to consume the generator to trigger the error | ||
| list(stub.streamingcall("some_method")) | ||
| assert "missing_driver" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_stub_driver_client_stream_raises(): | ||
| """Test that stream() raises ImportError with driver info.""" | ||
| stub = create_stub_client("missing_driver.client.Client") | ||
| with pytest.raises(ImportError) as exc_info: | ||
| with stub.stream(): | ||
| pass | ||
| assert "missing_driver" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_stub_driver_client_log_stream_raises(): | ||
| """Test that log_stream() raises ImportError with driver info.""" | ||
| stub = create_stub_client("missing_driver.client.Client") | ||
| with pytest.raises(ImportError) as exc_info: | ||
| with stub.log_stream(): | ||
| pass | ||
| assert "missing_driver" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_stub_driver_client_error_message_jumpstarter_driver(): | ||
| """Test that error message mentions version mismatch for Jumpstarter drivers.""" | ||
| stub = create_stub_client("jumpstarter_driver_xyz.client.XyzClient") | ||
| with pytest.raises(ImportError) as exc_info: | ||
| stub.call("some_method") | ||
| assert "version mismatch" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_stub_driver_client_error_message_third_party(): | ||
| """Test that error message includes install instructions for third-party drivers.""" | ||
| stub = create_stub_client("custom_driver.client.CustomClient") | ||
| with pytest.raises(ImportError) as exc_info: | ||
| stub.call("some_method") | ||
| assert "pip install custom_driver" in str(exc_info.value) |
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
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
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
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
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
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
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mangelajo @bennyz @bkhizgiy wdyt about this one? was suggested by the bot as sufficient, and simplified a bit instead of the previous "unsafe or in allow list" check. is it good enough?
on a side note, I'm thinking should we expose the exporter version to the client, and perhaps add some independent warning if it's different enough from the client one 🤔 maybe in a separate PR? this could eventually allow autoupdate if we decide to implement it, and address the issue from a different angle..
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to think about that,
If you can think about driver/client versioning, and adding some automated comparison.
I.e.
We should consider the typical semver
x.y.z
where change in x == incompatible
change in y == warning/recommend update ?
z == it's just a patch update and should not trigger even warnings.
Then it's up to the driver developer to bump versions accordingly, we should probably document this, and add a version check.
But it gets complicated, i.e. we cannot use the whole jumpstarter repo versioning, because we would be bumping the major version way too often without real reason.
May be we will have to collect driver versions too when we do jmp driver list, and also report the driver version on the exporter registration (another label)
But this is material for different issue/pr
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about something like exposing two parameters to the client, such as "exporter version", and "minimal recommended client version" while we can bump the former often (at each merge?) and latter one whenever a breaking change is being introduced 🤔 but yeah as you mentioned, there's the whole jumpstarter version vs separate driver versions to decide, and a few more complications, it's definitely not for the current PR :)
I guess I'll check what we have in issues, and if none, open a dedicated one.
back to the original question, would like to hear some opinions whatever
class_path.startswith("jumpstarter_driver_")is a sufficient check for "you should update the client" vs "should install a missing module with pip" messages wdyt?