Fix get_errors method #1213
Fix get_errors method #1213
Conversation
WalkthroughUpdated error traversal in get_errors to yield only the explicit cause chain, broadened the parameter type from Exception to BaseException, and extended DBus error handling to treat Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant get_errors as get_errors(exc: BaseException)
Note over Caller,get_errors #F0F7FF: Now yields initial exc, then follows only the explicit __cause__ chain.\nDBus ServiceUnknown is handled like NameHasNoOwner.
Caller->>get_errors: call with exc
activate get_errors
get_errors-->>Caller: yield initial exc
alt initial is DBus.ServiceUnknown
get_errors-->>Caller: interpret as "stratisd not running" message
else
loop for each explicit __cause__
get_errors->>get_errors: exc = exc.__cause__
get_errors-->>Caller: yield exc
end
end
deactivate get_errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/stratis_cli/_error_reporting.py (1)
82-92: Tiny polish: add return type and simplify loop.
- Add a return type for clarity.
- Slightly simpler loop reads cleaner.
- Optional: update nearby docstrings that say “list of Exception” to “list[BaseException]”.
Apply within this hunk:
-def get_errors(exc: BaseException): +def get_errors(exc: BaseException) -> "Iterator[BaseException]": @@ - while True: - yield exc - if exc.__cause__ is None: - return - exc = exc.__cause__ + while exc is not None: + yield exc + exc = exc.__cause__Add near imports:
from typing import Iterator
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/stratis_cli/_error_reporting.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-runs (3.13)
- GitHub Check: test-runs (3.12)
🔇 Additional comments (1)
src/stratis_cli/_error_reporting.py (1)
82-92: LGTM: cause-only traversal and widened type are correct.Following only
__cause__fixes noisy chains andBaseExceptionis appropriate.
mulkieran
force-pushed
the
fix-get-errors-bug
branch
from
6a11158 to
81df270
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/stratis_cli/_error_reporting.py (3)
82-90: Good fix; consider annotating return type and simplifying the loopThe behavioral change (follow only explicit cause) and widening to BaseException look right. Minor refinements:
- Add a return type to help callers and static tools.
- Simplify the loop to avoid repeating attribute access and to make intent clearer.
Apply:
-def get_errors(exc: BaseException): - """ - Generates a sequence of exceptions starting with exc and following the chain - of causes. - """ - yield exc - while exc.__cause__ is not None: - yield exc.__cause__ - exc = exc.__cause__ +def get_errors(exc: BaseException) -> "Iterator[BaseException]": + """ + Generates a sequence starting with exc and then following only the explicit + __cause__ chain (ignores __context__). + """ + while exc is not None: + yield exc + exc = exc.__cause__Add near other stdlib imports:
from typing import Iterator(Optional safety if you ever accept user-crafted exceptions that could form cycles):
seen = set() while exc is not None and id(exc) not in seen: seen.add(id(exc)) yield exc exc = exc.__cause__
169-172: Docstring types still say “list of Exception”; update to BaseExceptionFor consistency with get_errors(), update these docstrings to reflect BaseException.
- :type errors: list of Exception + :type errors: list[BaseException]Apply to:
- _interpret_errors_1 (Lines 169–172)
- _interpret_errors_2 (Lines 256–258)
- _interpret_errors (Lines 325–327)
Also applies to: 256-258, 325-327
346-347: Align handle_error param type with BaseExceptionDocstring still specifies Exception. Optionally annotate the function, too.
- :param Exception err: an exception + :param BaseException err: an exception(Optional signature annotation)
-def handle_error(err): +def handle_error(err: BaseException):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/stratis_cli/_error_reporting.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-runs (3.13)
- GitHub Check: test-runs (3.12)
🔇 Additional comments (1)
src/stratis_cli/_error_reporting.py (1)
82-90: No implicit context traversal detected—change is safe
Verified via ripgrep that neither production code nor tests reference__context__, and allget_errorsconsumers (including tests) operate solely on__cause__chains.
|
test failures must be infrastructure related |
mulkieran
force-pushed
the
fix-get-errors-bug
branch
from
81df270 to
1917075
Compare
|
Actually, fixing the get_errors method exposed a bug in the failing test. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/stratis_cli/_error_reporting.py(1 hunks)tests/whitebox/integration/test_stratis.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stratis_cli/_error_reporting.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T15:25:05.145Z
Learnt from: mulkieran
PR: stratis-storage/stratis-cli#0
File: :0-0
Timestamp: 2025-08-18T15:25:05.145Z
Learning: For the stratis-cli project, use Python's standard unittest framework instead of pytest. The codebase follows unittest.TestCase patterns with standard unittest assertions and test discovery.
Applied to files:
tests/whitebox/integration/test_stratis.py
🧬 Code graph analysis (1)
tests/whitebox/integration/test_stratis.py (1)
tests/whitebox/integration/_misc.py (1)
SimTestCase(217-242)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-runs (3.13)
- GitHub Check: test-runs (3.12)
| from stratis_cli._errors import StratisCliStratisdVersionError | ||
|
|
||
| from ._misc import RUNNER, TEST_RUNNER, RunTestCase, SimTestCase | ||
| from ._misc import RUNNER, TEST_RUNNER, SimTestCase |
There was a problem hiding this comment.
Propagate tests need RunTestCase; re-add import.
SimTestCase always starts the simulator (see tests/whitebox/integration/_misc.py Lines 216-241), so using it for propagation/error tests prevents the expected D-Bus failure. You’ll need RunTestCase imported again for the next change.
-from ._misc import RUNNER, TEST_RUNNER, SimTestCase
+from ._misc import RUNNER, TEST_RUNNER, RunTestCase, SimTestCase📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from ._misc import RUNNER, TEST_RUNNER, SimTestCase | |
| from ._misc import RUNNER, TEST_RUNNER, RunTestCase, SimTestCase |
🤖 Prompt for AI Agents
In tests/whitebox/integration/test_stratis.py around line 28, the RunTestCase
import was removed causing propagation tests to use SimTestCase (which starts
the simulator) and thus mask expected D-Bus failures; re-add RunTestCase to the
import list (e.g. import RunTestCase alongside RUNNER, TEST_RUNNER, SimTestCase)
so propagation/error tests use RunTestCase instead of SimTestCase.
|
|
||
|
|
||
| class PropagateTestCase(RunTestCase): | ||
| class PropagateTestCase(SimTestCase): |
There was a problem hiding this comment.
Revert base class to RunTestCase to actually exercise propagation-on-error.
With SimTestCase, daemon version succeeds and no dbus.exceptions.DBusException is raised, invalidating the test’s premise. Using RunTestCase restores the “no daemon running” scenario that triggers the error and validates --propagate.
As an alternative, keep SimTestCase but patch the D-Bus call to raise DBusException, decoupling the test from daemon presence.
-class PropagateTestCase(SimTestCase):
+class PropagateTestCase(RunTestCase):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class PropagateTestCase(SimTestCase): | |
| class PropagateTestCase(RunTestCase): |
🤖 Prompt for AI Agents
In tests/whitebox/integration/test_stratis.py around line 48, the test class
currently inherits SimTestCase which causes the daemon call to succeed and
prevents the expected dbus.exceptions.DBusException; change the base class to
RunTestCase so the test runs with no daemon and the propagation-on-error path is
exercised (also add or update the import for RunTestCase if missing).
Alternatively, if you prefer to keep SimTestCase, patch/mock the specific D-Bus
call used in the test to raise dbus.exceptions.DBusException so the --propagate
behavior is validated.
e9cae3d to
4d5ebe7
Compare
|
So, the error in get_errors() was not quite benign, it was hiding an omission in |
mulkieran
force-pushed
the
fix-get-errors-bug
branch
from
4d5ebe7 to
1589d1a
Compare
Signed-off-by: mulhern <amulhern@redhat.com>
There was a lot wrong with it, even though it has never, to my knowledge, failed. It used getattr incorrectly, without providing a default value of false, so that if the attribute was not present on the object, an AttributeError would be raised regardless. It would fall back on the context ancestor if there wasn't a cause, although I suspect it never did. This also was wrong, because the context would not give any help in determining the error message. Neither of these code defects is ever known to have caused a bug, but the ruff linter run by CodeRabbit objected to the construction, and it was correct. Signed-off-by: mulhern <amulhern@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/stratis_cli/_error_reporting.py (3)
93-96: Annotate return type for clarityThe function returns Optional[str]; add an annotation to align code and docstring.
Apply:
-def _interpret_errors_0( - error: dbus.exceptions.DBusException, -): +from typing import Optional + +def _interpret_errors_0( + error: dbus.exceptions.DBusException, +) -> Optional[str]:
125-128: Treating ServiceUnknown like NameHasNoOwner: LGTM; small readability tweakThe new mapping is appropriate. Minor readability/perf nit: cache the name and use a set; also narrow psutil iteration to name-only.
Apply:
- if error.get_dbus_name() in ( - "org.freedesktop.DBus.Error.NameHasNoOwner", - "org.freedesktop.DBus.Error.ServiceUnknown", - ): + name = error.get_dbus_name() + if name in { + "org.freedesktop.DBus.Error.NameHasNoOwner", + "org.freedesktop.DBus.Error.ServiceUnknown", + }: try: # pylint: disable=import-outside-toplevel # isort: THIRDPARTY import psutil - for proc in psutil.process_iter(): + for proc in psutil.process_iter(attrs=["name"]): try: - if proc.name() == "stratisd": # pragma: no cover + if proc.info.get("name") == "stratisd": # pragma: no cover return ( "Most likely stratis is unable to connect to the " "stratisd D-Bus service." )
82-90: Add return type annotation to get_errorsget_errors correctly follows the
__cause__chain, and noraisestatements without explicitfromwere found. Apply:+from typing import Iterator -def get_errors(exc: BaseException): +def get_errors(exc: BaseException) -> Iterator[BaseException]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/stratis_cli/_error_reporting.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-runs (3.13)
- GitHub Check: test-runs (3.12)
1 participant
Related #1207
Summary by CodeRabbit