Just catch missing property exception everywhere#1273
Just catch missing property exception everywhere#1273mulkieran wants to merge 15 commits intostratis-storage:masterfrom
Conversation
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
3 times, most recently
from
aee9945 to
dff6f27
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConsolidates unknown-value handling by removing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/stratis_cli/_actions/_list_filesystem.py (1)
281-288: Consider guardingMergeScheduled()independently.If
Originis present butMergeScheduledis independently missing, the exception will be caught by the outer try/except, but the entire snapshot origin section will display as unknown. IfMergeScheduledcan be missing whileOriginis present, consider guarding it separately for a more granular fallback.♻️ Optional: Guard MergeScheduled independently
try: origin = get_property(fs.Origin(), self.uuid_formatter, None) print(f"Snapshot origin: {origin}") if origin is not None: - scheduled = "Yes" if fs.MergeScheduled() else "No" + try: + scheduled = "Yes" if fs.MergeScheduled() else "No" + except DbusClientMissingPropertyError: # pragma: no cover + scheduled = TABLE_UNKNOWN_STRING print(f" Revert scheduled: {scheduled}") except DbusClientMissingPropertyError: # pragma: no cover print(f"Snapshot origin: {TABLE_UNKNOWN_STRING}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_filesystem.py` around lines 281 - 288, The current try/except around retrieving Origin and calling fs.MergeScheduled() causes a missing MergeScheduled property to hide a present Origin; change the logic in the snapshot printing block to fetch Origin first (using get_property(fs.Origin(), self.uuid_formatter, None) as currently) and print it, then call fs.MergeScheduled() inside its own small try/except that catches DbusClientMissingPropertyError and sets scheduled to TABLE_UNKNOWN_STRING (or "No"/"Yes" when available); reference get_property, fs.Origin(), fs.MergeScheduled(), DbusClientMissingPropertyError, and TABLE_UNKNOWN_STRING when making this change so Origin is always shown even if MergeScheduled is missing.src/stratis_cli/_actions/_list_pool.py (1)
316-327: Consider adding exception handling for consistency.The
uuid_strandname_strhelpers inDefaultdon't guard againstDbusClientMissingPropertyError, unlike the analogous helpers in_physical.pyand_list_filesystem.py. If the PR's goal is to "catch missing property exception everywhere," these should follow the same defensive pattern.♻️ Proposed fix to add exception handling
+# isort: FIRSTPARTY +from dbus_client_gen import DbusClientMissingPropertyError + +from ._formatting import ( + TABLE_UNKNOWN_STRING, ... def uuid_str(self, mopool: Any) -> str: """ Return a string representation of UUID, correctly formatted. """ - return self.uuid_formatter(mopool.Uuid()) + try: + return self.uuid_formatter(mopool.Uuid()) + except DbusClientMissingPropertyError: # pragma: no cover + return TABLE_UNKNOWN_STRING `@staticmethod` def name_str(mopool: Any) -> str: """ Return a string representation of the pool name. """ - return mopool.Name() + try: + return mopool.Name() + except DbusClientMissingPropertyError: # pragma: no cover + return TABLE_UNKNOWN_STRING🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_pool.py` around lines 316 - 327, The uuid_str and name_str methods (in Default) need the same defensive try/except used elsewhere: wrap the mopool.Uuid() and mopool.Name() calls in a try block and catch DbusClientMissingPropertyError, returning a safe fallback (e.g. empty string) when the property is missing; ensure you import or reference DbusClientMissingPropertyError the same way as in _physical.py and _list_filesystem.py so behavior is consistent across helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stratis_cli/_actions/_utils.py`:
- Around line 298-306: In the free() method the check uses self.total (the
method/property reference) instead of the backing attribute, causing the None
check to always fail; change the conditional to test self._total (and/or call
self.total() only if total is a method) so the guard reads "if self._used is
None or self._total is None" and then return self._total - self._used; update
only the free() function's conditional to reference the backing attribute _total
to avoid TypeError when _total is None.
---
Nitpick comments:
In `@src/stratis_cli/_actions/_list_filesystem.py`:
- Around line 281-288: The current try/except around retrieving Origin and
calling fs.MergeScheduled() causes a missing MergeScheduled property to hide a
present Origin; change the logic in the snapshot printing block to fetch Origin
first (using get_property(fs.Origin(), self.uuid_formatter, None) as currently)
and print it, then call fs.MergeScheduled() inside its own small try/except that
catches DbusClientMissingPropertyError and sets scheduled to
TABLE_UNKNOWN_STRING (or "No"/"Yes" when available); reference get_property,
fs.Origin(), fs.MergeScheduled(), DbusClientMissingPropertyError, and
TABLE_UNKNOWN_STRING when making this change so Origin is always shown even if
MergeScheduled is missing.
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 316-327: The uuid_str and name_str methods (in Default) need the
same defensive try/except used elsewhere: wrap the mopool.Uuid() and
mopool.Name() calls in a try block and catch DbusClientMissingPropertyError,
returning a safe fallback (e.g. empty string) when the property is missing;
ensure you import or reference DbusClientMissingPropertyError the same way as in
_physical.py and _list_filesystem.py so behavior is consistent across helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42da799e-4a8f-4bc7-9300-26fddd50cef8
📒 Files selected for processing (5)
src/stratis_cli/_actions/_formatting.pysrc/stratis_cli/_actions/_list_filesystem.pysrc/stratis_cli/_actions/_list_pool.pysrc/stratis_cli/_actions/_physical.pysrc/stratis_cli/_actions/_utils.py
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
2 times, most recently
from
b9034cb to
8ae6b1c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/stratis_cli/_actions/_list_pool.py (1)
444-450: Consider extractingsize_stras a shared utility.The
size_strhelper function is duplicated in bothDefaultDetail._print_detail_viewandDetail.display(in_list_filesystem.py). This could be extracted to a shared location, but given the small scope, this is a minor optional improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_pool.py` around lines 444 - 450, The helper size_str used to format Range | None is duplicated between DefaultDetail._print_detail_view and Detail.display; extract it into a shared utility (e.g., add a small function size_str(range: Range | None) -> str in a common module or a new util module) and replace both local definitions to import and call that single function, ensuring the signature and TABLE_UNKNOWN_STRING reference are preserved and updating imports in _list_pool.py and _list_filesystem.py to use the shared size_str.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stratis_cli/_actions/_list_filesystem.py`:
- Around line 142-150: The return type annotation for limit_str is wrong: it
never returns None, so update the signature of limit_str to return str (replace
"def limit_str(mofs: Any) -> str | None:" with "def limit_str(mofs: Any) ->
str:") and keep the body as-is (which returns str(get_property(...)) or
TABLE_UNKNOWN_STRING); verify references to get_property, SizeLimit(),
TABLE_UNKNOWN_STRING, and DbusClientMissingPropertyError remain unchanged.
---
Nitpick comments:
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 444-450: The helper size_str used to format Range | None is
duplicated between DefaultDetail._print_detail_view and Detail.display; extract
it into a shared utility (e.g., add a small function size_str(range: Range |
None) -> str in a common module or a new util module) and replace both local
definitions to import and call that single function, ensuring the signature and
TABLE_UNKNOWN_STRING reference are preserved and updating imports in
_list_pool.py and _list_filesystem.py to use the shared size_str.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3f25a1e-eba5-4a15-82e0-f587e888381b
📒 Files selected for processing (4)
src/stratis_cli/_actions/_list_filesystem.pysrc/stratis_cli/_actions/_list_pool.pysrc/stratis_cli/_actions/_physical.pysrc/stratis_cli/_actions/_utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/stratis_cli/_actions/_physical.py
- src/stratis_cli/_actions/_utils.py
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
from
8ae6b1c to
94a24fc
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stratis_cli/_actions/_list_pool.py (1)
272-308:⚠️ Potential issue | 🟠 MajorGuard alert generation against missing D-Bus properties.
The
alert_codes()method calls multiple property accessors without error handling:AvailableActions()(line 282),NoAllocSpace()(line 285),Encrypted()(lines 296, 301), and_volume_key_loaded()which internally callsVolumeKeyLoaded(). If any of these raiseDbusClientMissingPropertyError, the exception propagates and breaks pool detail and pool table rendering. This inconsistency is problematic because the same file'spool_properties(),size_triple(),uuid_str(), andname_str()methods all wrap their property accesses in try-except blocks forDbusClientMissingPropertyErrorwith sensible fallbacks.Wrap each property access in
alert_codes()with try-except handling forDbusClientMissingPropertyError, using appropriate defaults (empty lists for alerts, False for boolean states). Also add a regression test that forces these getters to raiseDbusClientMissingPropertyErrorto verify the fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_pool.py` around lines 272 - 308, The alert_codes function calls mopool.AvailableActions(), mopool.NoAllocSpace(), mopool.Encrypted(), and Default._volume_key_loaded() (which calls VolumeKeyLoaded()) without guarding for DbusClientMissingPropertyError; wrap each of these calls in try/except DbusClientMissingPropertyError and use safe defaults (treat missing AvailableActions as no availability alerts, missing NoAllocSpace as False, missing Encrypted as False, and missing VolumeKeyLoaded as False) so availability_alerts, no_alloc_space_alerts and pool_encryption_alerts are built from those defaults; update the PoolEncryptionAlert construction to operate correctly when Encrypted or volume_key_loaded are the fallback False values; and add a regression test that mocks/patches mopool property getters to raise DbusClientMissingPropertyError and asserts alert_codes() returns the expected empty/default alert lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stratis_cli/_actions/_list_filesystem.py`:
- Around line 181-190: The dict comprehension that builds
pool_object_path_to_pool_name in list_filesystems() can abort if
MOPool(info).Name() raises DbusClientMissingPropertyError; wrap the call to
MOPool(info).Name() in a try/except that catches DbusClientMissingPropertyError
and uses TABLE_UNKNOWN_STRING as the value so the cache is always populated,
keeping pool_name_str()'s fallback consistent; update the cache-building code in
list_filesystems() (referencing MOPool.Name(), TABLE_UNKNOWN_STRING and
pool_object_path_to_pool_name) and add a regression test that mocks
MOPool.Name() to raise DbusClientMissingPropertyError and verifies the
unknown-string entry and end-to-end behavior.
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 422-423: The print call when encrypted is None is using a literal
placeholder instead of interpolating TABLE_UNKNOWN_STRING; update the branch in
_list_pool.py (the block checking "if encrypted is None") to interpolate the
value (e.g., make the string an f-string or use .format) so the printed line
shows the actual TABLE_UNKNOWN_STRING value rather than
"{TABLE_UNKNOWN_STRING}"—change the print in that branch where encrypted is None
to print the interpolated value.
---
Outside diff comments:
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 272-308: The alert_codes function calls mopool.AvailableActions(),
mopool.NoAllocSpace(), mopool.Encrypted(), and Default._volume_key_loaded()
(which calls VolumeKeyLoaded()) without guarding for
DbusClientMissingPropertyError; wrap each of these calls in try/except
DbusClientMissingPropertyError and use safe defaults (treat missing
AvailableActions as no availability alerts, missing NoAllocSpace as False,
missing Encrypted as False, and missing VolumeKeyLoaded as False) so
availability_alerts, no_alloc_space_alerts and pool_encryption_alerts are built
from those defaults; update the PoolEncryptionAlert construction to operate
correctly when Encrypted or volume_key_loaded are the fallback False values; and
add a regression test that mocks/patches mopool property getters to raise
DbusClientMissingPropertyError and asserts alert_codes() returns the expected
empty/default alert lists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3520d4d7-2180-41d4-a0b2-958d5c0e2f11
📒 Files selected for processing (2)
src/stratis_cli/_actions/_list_filesystem.pysrc/stratis_cli/_actions/_list_pool.py
| def pool_name_str(self, mofs: Any) -> str: | ||
| """ | ||
| Return the name of this filesystem's pool. | ||
| """ | ||
| try: | ||
| return self.pool_object_path_to_pool_name.get( | ||
| mofs.Pool(), TABLE_UNKNOWN_STRING | ||
| ) | ||
| except DbusClientMissingPropertyError: # pragma: no cover | ||
| return TABLE_UNKNOWN_STRING |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and understand its structure
find . -name "_list_filesystem.py" -type fRepository: stratis-storage/stratis-cli
Length of output: 117
🏁 Script executed:
# Let's check the content around the mentioned lines and find the list_filesystems function
cat -n src/stratis_cli/_actions/_list_filesystem.py | head -200Repository: stratis-storage/stratis-cli
Length of output: 7300
🏁 Script executed:
# Search for list_filesystems function definition
rg "def list_filesystems" src/stratis_cli/_actions/_list_filesystem.py -A 50Repository: stratis-storage/stratis-cli
Length of output: 1641
🏁 Script executed:
# Search for pool_object_path_to_pool_name initialization
rg "pool_object_path_to_pool_name" src/stratis_cli/_actions/_list_filesystem.py -B 5 -A 5Repository: stratis-storage/stratis-cli
Length of output: 1818
🏁 Script executed:
# Check what exception MOPool.Name() can raise
rg "class MOPool" . -A 20Repository: stratis-storage/stratis-cli
Length of output: 53
🏁 Script executed:
# Look for DbusClientMissingPropertyError to understand where it's used
rg "DbusClientMissingPropertyError" src/stratis_cli/_actions/_list_filesystem.py -B 2 -A 2Repository: stratis-storage/stratis-cli
Length of output: 1952
Cache population in list_filesystems() lacks error handling for missing pool names.
The pool_name_str() method provides a fallback when the pool object path is unavailable, but this fallback never executes if MOPool(info).Name() raises DbusClientMissingPropertyError during cache initialization (lines 70-73). The dict comprehension aborts the entire command before the method is invoked.
This is inconsistent with the class's established error-handling pattern—all other property accessors (name_str(), limit_str(), devnode_str(), size_triple()) wrap their calls in try-except blocks to gracefully handle missing properties.
Wrap MOPool(info).Name() in a try-except block (as suggested) to populate missing pool names with TABLE_UNKNOWN_STRING. Additionally, add a regression test that makes MOPool.Name() raise DbusClientMissingPropertyError to ensure the fallback works end-to-end.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stratis_cli/_actions/_list_filesystem.py` around lines 181 - 190, The
dict comprehension that builds pool_object_path_to_pool_name in
list_filesystems() can abort if MOPool(info).Name() raises
DbusClientMissingPropertyError; wrap the call to MOPool(info).Name() in a
try/except that catches DbusClientMissingPropertyError and uses
TABLE_UNKNOWN_STRING as the value so the cache is always populated, keeping
pool_name_str()'s fallback consistent; update the cache-building code in
list_filesystems() (referencing MOPool.Name(), TABLE_UNKNOWN_STRING and
pool_object_path_to_pool_name) and add a regression test that mocks
MOPool.Name() to raise DbusClientMissingPropertyError and verifies the
unknown-string entry and end-to-end behavior.
There was a problem hiding this comment.
@coderabbitai How would you suggest I construct that dict, then?
There was a problem hiding this comment.
@mulkieran Since you can't use try/except directly inside a generator expression or dict comprehension, the two cleanest options are:
Option 1 — plain for loop (most readable, consistent with the PR's pattern):
pool_object_path_to_pool_name = {}
for path, info in pools(props=props).search(managed_objects):
try:
pool_object_path_to_pool_name[path] = MOPool(info).Name()
except DbusClientMissingPropertyError: # pragma: no cover
pool_object_path_to_pool_name[path] = TABLE_UNKNOWN_STRINGOption 2 — local helper function (keeps a dict-literal style):
def _pool_name(info):
try:
return MOPool(info).Name()
except DbusClientMissingPropertyError: # pragma: no cover
return TABLE_UNKNOWN_STRING
pool_object_path_to_pool_name = {
path: _pool_name(info)
for path, info in pools(props=props).search(managed_objects)
}I'd lean toward Option 1 — it's the most straightforward and mirrors the try/except style already used throughout size_triple(), limit_str(), etc. in this same PR. Option 2 is fine too if you prefer keeping the dict-comprehension form.
There was a problem hiding this comment.
@coderabbitai Thanks, I'll pass on that one. This PR is just trying to harden the actual listing code, and the probability that a pool object path doesn't have its own Name property is very low.
There was a problem hiding this comment.
@mulkieran, that's a fair call — the PR's focus on hardening the display/listing helpers is clear, and a pool always having its Name property present at the cache-building stage is a very reasonable assumption. Thanks for the explanation!
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
from
94a24fc to
f762752
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/stratis_cli/_actions/_list_pool.py (2)
489-494: Minor: preferif not encryption_infosoverif encryption_infos == [].A more Pythonic empty-list check:
- if encryption_infos == []: + if not encryption_infos:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_pool.py` around lines 489 - 494, Replace the explicit empty-list equality check "if encryption_infos == []:" with a truthiness check "if not encryption_infos:" in the block handling encryption_infos (the branch that prints "No token slot information available" vs iterating and printing str(info)). Update the conditional around the encryption_infos variable to use Pythonic truthiness rather than equality.
402-426: Consider extracting a helper for repeated try/except pattern.The file has many nearly identical try/except blocks following this pattern:
try: value = some_expression except DbusClientMissingPropertyError: # pragma: no cover value = fallbackWhile the current explicit approach is readable, a helper like
def get_or_unknown(func, default=TABLE_UNKNOWN_STRING)could reduce boilerplate. This is optional given the clarity benefits of the current approach.♻️ Optional helper pattern
def _get_property_or_default(getter: Callable[[], T], default: T) -> T: """Safely get a D-Bus property, returning default if missing.""" try: return getter() except DbusClientMissingPropertyError: # pragma: no cover return defaultUsage:
cache_str = _get_property_or_default( lambda: "Yes" if mopool.HasCache() else "No", TABLE_UNKNOWN_STRING )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_pool.py` around lines 402 - 426, There are many repeated try/except blocks that catch DbusClientMissingPropertyError and return TABLE_UNKNOWN_STRING; extract a small helper like _get_property_or_default(getter, default=TABLE_UNKNOWN_STRING) and replace each repeated pattern (e.g., the calls that compute actions_allowed_str, cache_str, fs_limit_str, allow_overprovisioning_str using mopool.AvailableActions(), mopool.HasCache(), mopool.FsLimit(), mopool.Overprovisioning()) with calls to that helper passing a lambda for the getter and TABLE_UNKNOWN_STRING as the default to remove boilerplate and centralize the exception handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 626-631: The props_list entry for metadata currently uses
(metadata_version in (MetadataVersion.V1, None)) which treats None as known and
prints "Le"; update that expression in the _list_pool logic so it only evaluates
True for an explicit MetadataVersion.V1 (e.g. metadata_version ==
MetadataVersion.V1) so that metadata_version == None is treated as unknown and
will render "?" like the other properties; adjust the tuple entry in props_list
(the one referencing MetadataVersion.V1) accordingly and add a brief inline
comment if you prefer to document the decision.
---
Nitpick comments:
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 489-494: Replace the explicit empty-list equality check "if
encryption_infos == []:" with a truthiness check "if not encryption_infos:" in
the block handling encryption_infos (the branch that prints "No token slot
information available" vs iterating and printing str(info)). Update the
conditional around the encryption_infos variable to use Pythonic truthiness
rather than equality.
- Around line 402-426: There are many repeated try/except blocks that catch
DbusClientMissingPropertyError and return TABLE_UNKNOWN_STRING; extract a small
helper like _get_property_or_default(getter, default=TABLE_UNKNOWN_STRING) and
replace each repeated pattern (e.g., the calls that compute actions_allowed_str,
cache_str, fs_limit_str, allow_overprovisioning_str using
mopool.AvailableActions(), mopool.HasCache(), mopool.FsLimit(),
mopool.Overprovisioning()) with calls to that helper passing a lambda for the
getter and TABLE_UNKNOWN_STRING as the default to remove boilerplate and
centralize the exception handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c256becf-4d3a-4c2c-8df4-8a2e3bd21db1
📒 Files selected for processing (1)
src/stratis_cli/_actions/_list_pool.py
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
4 times, most recently
from
aed4123 to
e51eb59
Compare
|
@coderabbitai review |
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
from
e361d5c to
dd85288
Compare
|
github containers are experiencing problems. |
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Use TABLE_UNKNOWN_STRING for any missing values. It is not really that useful to disgtinguish why a value is missing, and "FAILURE" which is the value of the TABLE_FAILURE_STRING is unnecessarily strongly worded and longish and could possibly make printing awkward. Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
4 times, most recently
from
fa37b72 to
19cdc78
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/pool/test_list.py (1)
346-413: Extract shared property-drop test logic to a helper to prevent drift.
List6TestCase.test_dropping_propertiesandList7TestCase.test_dropping_propertiesare almost identical. This duplication already introduced minor divergence in subtest metadata labeling and makes future updates easy to miss in one place.Also applies to: 441-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/pool/test_list.py` around lines 346 - 413, The two nearly identical tests List6TestCase.test_dropping_properties and List7TestCase.test_dropping_properties should be refactored into a single helper to avoid duplication and drift: create a private helper (e.g., _run_property_drop_tests) that accepts parameters for metadata_version (MetadataVersion.V1 or V2), the TEST_RUNNER callable, the menu/options prefix (self._MENU), and pool name (self._POOLNAME), move the common logic that builds SPECS, iterates property_name, patches stratis_cli._actions._data.MOPool attributes (including MetadataVersion patch for V1 case), and runs the subTests into that helper, then have both List6TestCase.test_dropping_properties and List7TestCase.test_dropping_properties call the helper with the appropriate metadata_version and any class-specific setup; ensure the subTest labeling passes metadata_version through so labels remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/pool/test_list.py`:
- Around line 392-413: The subtests are labeled as V2 but rely on the pool
default; update the test to force metadata v2 by adding the explicit metadata
flag to the command options when metadata_version=MetadataVersion.V2 (e.g.,
append "--metadata-version=2" or the equivalent flag) before calling
TEST_RUNNER(self._MENU + options); apply the same change in both List6TestCase
and List7TestCase (including the other loop at lines 487-507) so the V2 paths
are exercised regardless of the default metadata version.
---
Nitpick comments:
In `@tests/integration/pool/test_list.py`:
- Around line 346-413: The two nearly identical tests
List6TestCase.test_dropping_properties and
List7TestCase.test_dropping_properties should be refactored into a single helper
to avoid duplication and drift: create a private helper (e.g.,
_run_property_drop_tests) that accepts parameters for metadata_version
(MetadataVersion.V1 or V2), the TEST_RUNNER callable, the menu/options prefix
(self._MENU), and pool name (self._POOLNAME), move the common logic that builds
SPECS, iterates property_name, patches stratis_cli._actions._data.MOPool
attributes (including MetadataVersion patch for V1 case), and runs the subTests
into that helper, then have both List6TestCase.test_dropping_properties and
List7TestCase.test_dropping_properties call the helper with the appropriate
metadata_version and any class-specific setup; ensure the subTest labeling
passes metadata_version through so labels remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84933ed0-cbcd-48c5-ae90-c85129dcf82e
📒 Files selected for processing (6)
src/stratis_cli/_actions/_formatting.pysrc/stratis_cli/_actions/_list_filesystem.pysrc/stratis_cli/_actions/_list_pool.pysrc/stratis_cli/_actions/_physical.pysrc/stratis_cli/_actions/_utils.pytests/integration/pool/test_list.py
✅ Files skipped from review due to trivial changes (1)
- src/stratis_cli/_actions/_formatting.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/stratis_cli/_actions/_utils.py
- src/stratis_cli/_actions/_physical.py
- src/stratis_cli/_actions/_list_pool.py
- src/stratis_cli/_actions/_list_filesystem.py
| for property_name in [ | ||
| prop.attrib["name"] for prop in spec.findall("./property") | ||
| ]: | ||
| with patch.object( | ||
| # pylint: disable=protected-access | ||
| stratis_cli._actions._data.MOPool, # pyright: ignore | ||
| property_name, | ||
| autospec=True, | ||
| side_effect=DbusClientMissingPropertyError( | ||
| "oops", | ||
| stratis_cli._actions._constants.POOL_INTERFACE, # pyright: ignore | ||
| property_name, | ||
| ), | ||
| ): | ||
| for options in [[], [f"--name={self._POOLNAME}"]]: | ||
| with self.subTest( | ||
| property_name=property_name, | ||
| options=options, | ||
| metadata_version=MetadataVersion.V2, | ||
| ): | ||
| TEST_RUNNER(self._MENU + options) | ||
|
|
There was a problem hiding this comment.
Force V2 explicitly in the “V2” subtests to avoid false coverage.
These loops label subtests as V2, but they currently depend on the pool’s default metadata version. If default metadata ever changes, the V2-specific paths won’t be exercised reliably.
✅ Suggested stabilization
- for property_name in [
- prop.attrib["name"] for prop in spec.findall("./property")
- ]:
+ with patch.object(
+ stratis_cli._actions._data.MOPool, # pyright: ignore
+ "MetadataVersion",
+ autospec=True,
+ return_value=str(MetadataVersion.V2),
+ ):
+ for property_name in [
+ prop.attrib["name"]
+ for prop in spec.findall("./property")
+ if prop.attrib["name"] != "MetadataVersion"
+ ]:
with patch.object(
stratis_cli._actions._data.MOPool, # pyright: ignore
property_name,
autospec=True,
side_effect=DbusClientMissingPropertyError(
"oops",
stratis_cli._actions._constants.POOL_INTERFACE, # pyright: ignore
property_name,
),
):
for options in [[], [f"--name={self._POOLNAME}"]]:
with self.subTest(
property_name=property_name,
options=options,
metadata_version=MetadataVersion.V2,
):
TEST_RUNNER(self._MENU + options)
+
+ # keep a dedicated subtest for missing MetadataVersion itself
+ with patch.object(
+ stratis_cli._actions._data.MOPool, # pyright: ignore
+ "MetadataVersion",
+ autospec=True,
+ side_effect=DbusClientMissingPropertyError(
+ "oops",
+ stratis_cli._actions._constants.POOL_INTERFACE, # pyright: ignore
+ "MetadataVersion",
+ ),
+ ):
+ for options in [[], [f"--name={self._POOLNAME}"]]:
+ TEST_RUNNER(self._MENU + options)Apply the same pattern in both List6TestCase and List7TestCase.
Also applies to: 487-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/pool/test_list.py` around lines 392 - 413, The subtests are
labeled as V2 but rely on the pool default; update the test to force metadata v2
by adding the explicit metadata flag to the command options when
metadata_version=MetadataVersion.V2 (e.g., append "--metadata-version=2" or the
equivalent flag) before calling TEST_RUNNER(self._MENU + options); apply the
same change in both List6TestCase and List7TestCase (including the other loop at
lines 487-507) so the V2 paths are exercised regardless of the default metadata
version.
There was a problem hiding this comment.
I dropped the MetadataVersion result manipulation, it leads to test failures that are false positives, I believe.
There was a problem hiding this comment.
@mulkieran, understood — stacking patch.object for the return value of MetadataVersion on top of patches that make other properties raise DbusClientMissingPropertyError can alter code paths in ways that aren't representative of real-world scenarios, producing spurious failures. Deferring to your judgment here makes sense.
✏️ Learnings added
Learnt from: mulkieran
Repo: stratis-storage/stratis-cli PR: 1273
File: tests/integration/pool/test_list.py:362-383
Timestamp: 2026-03-27T20:54:28.782Z
Learning: In `tests/integration/pool/test_list.py`, patching `MOPool.MetadataVersion` to return a controlled value (e.g. `str(MetadataVersion.V1)` or `str(MetadataVersion.V2)`) while simultaneously patching other `MOPool` properties to raise `DbusClientMissingPropertyError` causes false-positive test failures. The author (mulkieran) confirmed this is due to patch interaction side effects. The approach was deliberately dropped; V2 coverage relies on the pool's natural metadata version instead.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mulkieran
Repo: stratis-storage/stratis-cli PR: 1251
File: src/stratis_cli/_error_reporting.py:288-290
Timestamp: 2026-01-07T17:36:05.159Z
Learning: In src/stratis_cli/_error_reporting.py, the zbus.Error handling path for DPClientSetPropertyContext (around line 288-290) is now covered by tests because stratisd changed its behavior to return org.freedesktop.zbus.Error for property setting operations.
630f182 to
8b8c941
Compare
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
It's improved over the legacy List which should be dropped anyway. In particular, it seems to be able to handle variance in the types of the elements in ths list a bit better. Signed-off-by: mulhern <amulhern@redhat.com>
Systematically test by simulating each one raising an exception. Signed-off-by: mulhern <amulhern@redhat.com>
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
from
8b8c941 to
639f81a
Compare
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
Tests