| Message ID | 20191217203155.24206-5-marcos.souza.org@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests: do not fail if dm-thin is missing | expand |
On 2019/12/18 上午4:31, Marcos Paulo de Souza wrote: > From: Marcos Paulo de Souza <mpdesouza@suse.com> > > Move the check of dmsetup to check_dm_target_support, and adapt the only > two places checking if dmsetup is present in the system. Now we skip the > test if dmsetup isn't available, instead of marking the test as failed. > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> Looks good overall, just a small nitpick inlined below. > --- > tests/common | 9 +++++++-- > tests/mkfs-tests/005-long-device-name-for-ssd/test.sh | 1 - > .../017-small-backing-size-thin-provision-device/test.sh | 1 - > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/tests/common b/tests/common > index f138b17e..dc2d084e 100644 > --- a/tests/common > +++ b/tests/common > @@ -322,10 +322,15 @@ check_global_prereq() > fi > } > > -# check if the targets passed as arguments are available, and if not just skip > -# the test > +# check if dmsetup and targets passed as arguments are available, and skip the > +# test if they aren't. > check_dm_target_support() > { > + which dmsetup &> /dev/null > + if [ $? -ne 0 ]; then > + _not_run "This test requires dmsetup tool."; > + fi What about using existing check_global_prereq()? Despite that, Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > + > for target in "$@"; do > $SUDO_HELPER modprobe dm-$target >/dev/null 2>&1 > $SUDO_HELPER dmsetup targets 2>&1 | grep -q ^$target > diff --git a/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh b/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh > index 329deaf2..2df88db4 100755 > --- a/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh > +++ b/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh > @@ -4,7 +4,6 @@ > source "$TEST_TOP/common" > > check_prereq mkfs.btrfs > -check_global_prereq dmsetup > check_dm_target_support linear > > setup_root_helper > diff --git a/tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh b/tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh > index 91851945..83f34ecc 100755 > --- a/tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh > +++ b/tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh > @@ -6,7 +6,6 @@ source "$TEST_TOP/common" > > check_prereq mkfs.btrfs > check_global_prereq udevadm > -check_global_prereq dmsetup > check_dm_target_support linear thin > > setup_root_helper >
On Wed, 2019-12-18 at 08:30 +0800, Qu Wenruo wrote: > > On 2019/12/18 上午4:31, Marcos Paulo de Souza wrote: > > From: Marcos Paulo de Souza <mpdesouza@suse.com> > > > > Move the check of dmsetup to check_dm_target_support, and adapt the > only > > two places checking if dmsetup is present in the system. Now we > skip the > > test if dmsetup isn't available, instead of marking the test as > failed. > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > Looks good overall, just a small nitpick inlined below. > > > --- > > tests/common | 9 > +++++++-- > > tests/mkfs-tests/005-long-device-name-for-ssd/test.sh | 1 - > > .../017-small-backing-size-thin-provision-device/test.sh | 1 - > > 3 files changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/tests/common b/tests/common > > index f138b17e..dc2d084e 100644 > > --- a/tests/common > > +++ b/tests/common > > @@ -322,10 +322,15 @@ check_global_prereq() > > fi > > } > > > > -# check if the targets passed as arguments are available, and if > not just skip > > -# the test > > +# check if dmsetup and targets passed as arguments are available, > and skip the > > +# test if they aren't. > > check_dm_target_support() > > { > > + which dmsetup &> /dev/null > > + if [ $? -ne 0 ]; then > > + _not_run "This test requires dmsetup tool."; > > + fi > > What about using existing check_global_prereq()? check_global_prereq call _fail when the executable is not found in $PATH, that's why I open coded the implementation and just called _not_run. > > Despite that, > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Thanks, > Qu > > > + > > for target in "$@"; do > > $SUDO_HELPER modprobe dm-$target >/dev/null 2>&1 > > $SUDO_HELPER dmsetup targets 2>&1 | grep -q ^$target > > diff --git a/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh > b/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh > > index 329deaf2..2df88db4 100755 > > --- a/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh > > +++ b/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh > > @@ -4,7 +4,6 @@ > > source "$TEST_TOP/common" > > > > check_prereq mkfs.btrfs > > -check_global_prereq dmsetup > > check_dm_target_support linear > > > > setup_root_helper > > diff --git a/tests/mkfs-tests/017-small-backing-size-thin- > provision-device/test.sh b/tests/mkfs-tests/017-small-backing-size- > thin-provision-device/test.sh > > index 91851945..83f34ecc 100755 > > --- a/tests/mkfs-tests/017-small-backing-size-thin-provision- > device/test.sh > > +++ b/tests/mkfs-tests/017-small-backing-size-thin-provision- > device/test.sh > > @@ -6,7 +6,6 @@ source "$TEST_TOP/common" > > > > check_prereq mkfs.btrfs > > check_global_prereq udevadm > > -check_global_prereq dmsetup > > check_dm_target_support linear thin > > > > setup_root_helper > > >
On 2019/12/18 上午8:44, Marcos Paulo de Souza wrote: > On Wed, 2019-12-18 at 08:30 +0800, Qu Wenruo wrote: >> >> On 2019/12/18 上午4:31, Marcos Paulo de Souza wrote: >>> From: Marcos Paulo de Souza <mpdesouza@suse.com> >>> >>> Move the check of dmsetup to check_dm_target_support, and adapt the >> only >>> two places checking if dmsetup is present in the system. Now we >> skip the >>> test if dmsetup isn't available, instead of marking the test as >> failed. >>> >>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> >> >> Looks good overall, just a small nitpick inlined below. >> >>> --- >>> tests/common | 9 >> +++++++-- >>> tests/mkfs-tests/005-long-device-name-for-ssd/test.sh | 1 - >>> .../017-small-backing-size-thin-provision-device/test.sh | 1 - >>> 3 files changed, 7 insertions(+), 4 deletions(-) >>> >>> diff --git a/tests/common b/tests/common >>> index f138b17e..dc2d084e 100644 >>> --- a/tests/common >>> +++ b/tests/common >>> @@ -322,10 +322,15 @@ check_global_prereq() >>> fi >>> } >>> >>> -# check if the targets passed as arguments are available, and if >> not just skip >>> -# the test >>> +# check if dmsetup and targets passed as arguments are available, >> and skip the >>> +# test if they aren't. >>> check_dm_target_support() >>> { >>> + which dmsetup &> /dev/null >>> + if [ $? -ne 0 ]; then >>> + _not_run "This test requires dmsetup tool."; >>> + fi >> >> What about using existing check_global_prereq()? > > check_global_prereq call _fail when the executable is not found in > $PATH, that's why I open coded the implementation and just called > _not_run. Makes sense. Although it would be even better to change from "_fail" to "_not_run" for check_global_prereq(). That could be a new patch. Thanks, Qu > >> >> Despite that, >> >> Reviewed-by: Qu Wenruo <wqu@suse.com> >> >> Thanks, >> Qu >> >>> + >>> for target in "$@"; do >>> $SUDO_HELPER modprobe dm-$target >/dev/null 2>&1 >>> $SUDO_HELPER dmsetup targets 2>&1 | grep -q ^$target >>> diff --git a/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh >> b/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh >>> index 329deaf2..2df88db4 100755 >>> --- a/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh >>> +++ b/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh >>> @@ -4,7 +4,6 @@ >>> source "$TEST_TOP/common" >>> >>> check_prereq mkfs.btrfs >>> -check_global_prereq dmsetup >>> check_dm_target_support linear >>> >>> setup_root_helper >>> diff --git a/tests/mkfs-tests/017-small-backing-size-thin- >> provision-device/test.sh b/tests/mkfs-tests/017-small-backing-size- >> thin-provision-device/test.sh >>> index 91851945..83f34ecc 100755 >>> --- a/tests/mkfs-tests/017-small-backing-size-thin-provision- >> device/test.sh >>> +++ b/tests/mkfs-tests/017-small-backing-size-thin-provision- >> device/test.sh >>> @@ -6,7 +6,6 @@ source "$TEST_TOP/common" >>> >>> check_prereq mkfs.btrfs >>> check_global_prereq udevadm >>> -check_global_prereq dmsetup >>> check_dm_target_support linear thin >>> >>> setup_root_helper >>> >> >
On Wed, Dec 18, 2019 at 08:48:21AM +0800, Qu Wenruo wrote: > >>> +# check if dmsetup and targets passed as arguments are available, > >> and skip the > >>> +# test if they aren't. > >>> check_dm_target_support() > >>> { > >>> + which dmsetup &> /dev/null > >>> + if [ $? -ne 0 ]; then > >>> + _not_run "This test requires dmsetup tool."; > >>> + fi > >> > >> What about using existing check_global_prereq()? > > > > check_global_prereq call _fail when the executable is not found in > > $PATH, that's why I open coded the implementation and just called > > _not_run. > > Makes sense. > > Although it would be even better to change from "_fail" to "_not_run" > for check_global_prereq(). I'd rather keep it as _fail, the utilities checked by this helper is mostly some system tool or other filesystem tool, all should be present for the testing. I don't want the testsuite to skip random tests without a good reason. We can add a script or make target to make the checks in advance and not when some test fails after a long time.
diff --git a/tests/common b/tests/common index f138b17e..dc2d084e 100644 --- a/tests/common +++ b/tests/common @@ -322,10 +322,15 @@ check_global_prereq() fi } -# check if the targets passed as arguments are available, and if not just skip -# the test +# check if dmsetup and targets passed as arguments are available, and skip the +# test if they aren't. check_dm_target_support() { + which dmsetup &> /dev/null + if [ $? -ne 0 ]; then + _not_run "This test requires dmsetup tool."; + fi + for target in "$@"; do $SUDO_HELPER modprobe dm-$target >/dev/null 2>&1 $SUDO_HELPER dmsetup targets 2>&1 | grep -q ^$target diff --git a/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh b/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh index 329deaf2..2df88db4 100755 --- a/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh +++ b/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh @@ -4,7 +4,6 @@ source "$TEST_TOP/common" check_prereq mkfs.btrfs -check_global_prereq dmsetup check_dm_target_support linear setup_root_helper diff --git a/tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh b/tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh index 91851945..83f34ecc 100755 --- a/tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh +++ b/tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh @@ -6,7 +6,6 @@ source "$TEST_TOP/common" check_prereq mkfs.btrfs check_global_prereq udevadm -check_global_prereq dmsetup check_dm_target_support linear thin setup_root_helper