Various test fixes #1162
Various test fixes #1162
Conversation
Too much copy-pasting.
Same implementation in both plugins.
Checking only EUID is not enough and we don't really support running without root privileges. See 80ba38c for details.
The generic XFS resize and info function should work with both and mount an unmounted device when needed.
📝 WalkthroughWalkthroughAdded LCOV documentation; stricter fstype validation in FS checks; moved/added and removed LVM cache-stats implementations across backends; adjusted SMART API and added shim; removed root-only guards in multipath DM ops; and introduced many tests across plugins and utils. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/smart/libatasmart.c (1)
56-66: Handle combinedBDSmartTechflags explicitly.
BDSmartTechis currently used with combined flags (e.g.,BD_SMART_TECH_ATA | BD_SMART_TECH_SCSIinsmartmontools.c), but the enum values are sequential (0, 1) rather than powers of 2. Theswitchstatement relies on the coincidence that0 | 1 = 1, which happens to matchBD_SMART_TECH_SCSI. This works by accident but is fragile and error-prone. Use bitmask checks to handle combined flags explicitly and correctly.🔧 Suggested fix
-gboolean bd_smart_is_tech_avail (BDSmartTech tech, G_GNUC_UNUSED guint64 mode, GError **error) { - switch (tech) { - case BD_SMART_TECH_ATA: - return TRUE; - case BD_SMART_TECH_SCSI: - g_set_error (error, BD_SMART_ERROR, BD_SMART_ERROR_TECH_UNAVAIL, "SCSI SMART is unavailable with libatasmart"); - return FALSE; - default: - g_set_error (error, BD_SMART_ERROR, BD_SMART_ERROR_TECH_UNAVAIL, "Unknown technology"); - return FALSE; - } -} +gboolean bd_smart_is_tech_avail (BDSmartTech tech, G_GNUC_UNUSED guint64 mode, GError **error) { + if (tech & BD_SMART_TECH_SCSI) { + g_set_error (error, BD_SMART_ERROR, BD_SMART_ERROR_TECH_UNAVAIL, + "SCSI SMART is unavailable with libatasmart"); + return FALSE; + } + if (tech & BD_SMART_TECH_ATA) + return TRUE; + + g_set_error (error, BD_SMART_ERROR, BD_SMART_ERROR_TECH_UNAVAIL, "Unknown technology"); + return FALSE; +}
🤖 Fix all issues with AI agents
In `@tests/crypto_test.py`:
- Around line 589-595: After calling
BlockDev.crypto_luks_add_key(self.loop_devs[0], ctx, nctx2) the test assigns the
result to succ but never asserts it; add a verification similar to the
subsequent call to ensure the operation succeeded. Update tests/crypto_test.py
to assert succ (e.g., self.assertTrue(succ)) immediately after the first
crypto_luks_add_key call that produces succ so failures adding the keyfile-based
key are caught; reference the existing BlockDev.CryptoKeyslotContext and
BlockDev.crypto_luks_add_key usage and the succ variable for placement.
🧹 Nitpick comments (6)
tests/btrfs_test.py (1)
382-382: Consider usingos.makedirs()for consistency and safety.While
TEST_MNTis a hardcoded constant (so the shell injection risk flagged by static analysis is a false positive in practice), usingos.makedirs()would be more Pythonic and consistent with line 388 which already uses this approach.Suggested change
- os.system("mkdir -p %s/one/two/one/two" % (TEST_MNT)) + os.makedirs(os.path.join(TEST_MNT, "one/two/one/two"), exist_ok=True)tests/mpath_test.py (1)
51-55: Avoid unnecessary storage setup for this no-op test.This test doesn’t use the loop device from
MpathTestCase.setUp, so it pays the LIO setup cost (and potential privilege failures) unnecessarily. Consider moving it toMpathNoDevTestCaseand tagging itNOSTORAGEto keep it runnable in minimal environments.♻️ Proposed refactor
-class MpathTestCase(MpathTest): +class MpathTestCase(MpathTest): @@ - def test_flush_mpaths(self): - """ Verify that mpath_flush_mpaths can be called """ - # we have no multipath devices, but flush should still work (and do nothing) - succ = BlockDev.mpath_flush_mpaths() - self.assertTrue(succ) + # (moved to MpathNoDevTestCase) @@ -class MpathNoDevTestCase(MpathTest): +class MpathNoDevTestCase(MpathTest): + `@tag_test`(TestTags.NOSTORAGE) + def test_flush_mpaths(self): + """ Verify that mpath_flush_mpaths can be called """ + # we have no multipath devices, but flush should still work (and do nothing) + succ = BlockDev.mpath_flush_mpaths() + self.assertTrue(succ)tests/utils_test.py (1)
538-554: Consider potential test flakiness.The test assumes that if no loop devices exist (
/dev/loop[0-9]*), the loop module can be safely loaded and unloaded. However:
- The module might already be loaded but with no active loop devices
- The module could be built into the kernel rather than loadable
utils_unload_kernel_modulemay fail if the module is in use by other subsystemsThe conditional guard helps, but this test might still fail in certain environments. Consider adding the
TestTags.UNSTABLEtag if flakiness is observed in CI.tests/fs_tests/generic_test.py (1)
959-967: Rename the UUID failure test for clarity.
The method name says “label” but it tests UUID behavior, which can confuse future readers.♻️ Suggested rename for clarity
- def test_fail_generic_check_label(self): + def test_fail_generic_check_uuid(self): @@ - BlockDev.fs_check_uuid("", "label") + BlockDev.fs_check_uuid("", "uuid")src/plugins/smart/smartmontools.c (1)
54-58: Avoid bitwise OR onBDSmartTech.
BDSmartTechis an enum, not a flags type. PassingATA | SCSIis non-idiomatic and could break ifbd_smart_is_tech_availstarts using the tech parameter in this plugin.♻️ Suggested refactor
- return bd_smart_is_tech_avail (BD_SMART_TECH_ATA | BD_SMART_TECH_SCSI, 0, NULL); + return bd_smart_is_tech_avail (BD_SMART_TECH_ATA, 0, NULL) && + bd_smart_is_tech_avail (BD_SMART_TECH_SCSI, 0, NULL);tests/lvm_dbus_tests.py (1)
72-82: Add reinit after fake_path context to restore normal tool availability state.Inside the
fake_pathcontext,BlockDev.reinit()reloads the plugin with a restricted PATH, marking tools (lvmconfig, lvmdevices) as unavailable. When the context exits, PATH is restored but the plugin's cached dependency state persists, potentially affecting subsequent tests that rely on tool availability. Add a reinit call after the context to reload with the normal PATH restored.Suggested fix
with fake_path(all_but=("lvmconfig", "lvmdevices")): self.assertTrue(BlockDev.reinit(self.requested_plugins, True, None)) # no lvmconfig tool available, should fail with self.assertRaises(GLib.GError): BlockDev.lvm_is_tech_avail(BlockDev.LVMTech.CONFIG, 0) # no lvmdevices tool available, should fail with self.assertRaises(GLib.GError): BlockDev.lvm_is_tech_avail(BlockDev.LVMTech.DEVICES, 0) + + # Restore normal PATH and refresh plugin state + self.assertTrue(BlockDev.reinit(self.requested_plugins, True, None))
vojtechtrefny
force-pushed
the
master_test-fixes-2
branch
from
8b488c8 to
a4c2bcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/fs_tests/generic_test.py`:
- Around line 849-860: The skip message in test_vfat_generic_set_label is
incorrect: change the text passed to self.skipTest from "dosfstools >= 4.2
needed to set UUID" to a message that references labels (e.g. "dosfstools >= 4.2
needed to set labels" or "dosfstools >= 4.2 needed to set vfat labels") so the
skip message matches the test's purpose; update the string in the
test_vfat_generic_set_label function where mkfs_vfat and _test_generic_set_label
are used.
In `@tests/utils_test.py`:
- Around line 561-563: The tearDown method is removing the GLib log handler with
an empty string domain which doesn't match how the handler was registered
(registered with None); update the removal call in tearDown to use the same
domain value (None) as used when setting the handler so
GLib.log_remove_handler(None, self.handler) matches the original
GLib.log_set_handler/GLib.log_add handler registration (refer to the tearDown
method and the code that registers the handler).
🧹 Nitpick comments (3)
tests/mpath_test.py (1)
51-56: Consider moving this test toMpathNoDevTestCase.This test doesn't use the loop device created in
MpathTestCase.setUp(). Since it only verifies thatmpath_flush_mpaths()can be called when no multipath devices exist, it could be moved toMpathNoDevTestCaseand tagged with@tag_test(TestTags.NOSTORAGE)to avoid unnecessary device setup/teardown overhead.♻️ Suggested refactor
class MpathNoDevTestCase(MpathTest): `@tag_test`(TestTags.NOSTORAGE) def test_plugin_version(self): self.assertEqual(BlockDev.get_plugin_soname(BlockDev.Plugin.MPATH), "libbd_mpath.so.3") + `@tag_test`(TestTags.NOSTORAGE) + def test_flush_mpaths(self): + """ Verify that mpath_flush_mpaths can be called """ + # we have no multipath devices, but flush should still work (and do nothing) + succ = BlockDev.mpath_flush_mpaths() + self.assertTrue(succ) + `@tag_test`(TestTags.NOSTORAGE) def test_get_mpath_members(self):tests/utils_test.py (1)
565-566: Unused callback parameters are required by GLib signature.The static analysis flags
domain,level, andusrptras unused, but these parameters are required by the GLib log handler callback signature. This is a false positive.Consider adding a brief comment or using underscore prefixes to document the intent:
Optional: clarify unused parameters
- def log_func(self, domain, level, msg, usrptr): + def log_func(self, _domain, _level, msg, _usrptr): self.log += msgtests/fs_tests/generic_test.py (1)
1062-1175: Consider a small helper to reduce repeated mount branching.
The repeatedif mount/with mounted(...)blocks are easy to drift; a tiny wrapper would keep this concise.
vojtechtrefny
force-pushed
the
master_test-fixes-2
branch
from
a4c2bcf to
75b4720
Compare
src/plugins/fs/generic.c
Outdated
| */ | ||
| gboolean bd_fs_check_label (const gchar *fstype, const gchar *label, GError **error) { | ||
| if (!fstype) { | ||
| if (!fstype || g_strcmp0 (fstype, "") == 0) { |
There was a problem hiding this comment.
Am I being too optimistic if I think this doesn't matter because the compiler will optimize this?
There was a problem hiding this comment.
Hmmm, probably not since this is a cross-library call. If it were the plain strcmp(), then perhaps.
Additional coverage for logging, kernel module (un)loading etc.
Checking only EUID is not enough and we don't really support running without root privileges. See 80ba38c for details.
This function should've never been added -- the check_deps functions were removed in 3.0 and are no longer needed. bd_smart_check_deps now calls bd_smart_is_tech_avail which is also the recommended replacement. This function was also not previously implemented in the libatasmart plugin, which is also incorrect, all implementations must implement all functions.
vojtechtrefny
force-pushed
the
master_test-fixes-2
branch
from
75b4720 to
cb4190b
Compare
2 participants
Mostly prompted by our QE's coverage report. I am definitely not aiming for 100 % coverage, but there were some quite bad "holes" in our tests, sometimes even hiding some real issues.
Just for fun: LCOV report on master and with these changes. The difference isn't that huge (lines coverage from 62.6 % to 64.3 % and function coverage from 65.7 % to 67.5 %), but most of the "missing parts" are either error cases or the copy and free functions that are not really covered by the python tests.
Summary by CodeRabbit
Bug Fixes
Documentation
Public API
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.