[btrfs-progs,4/4] tests: Do not fail is dmsetup is missing
diff mbox series

Message ID 20191217203155.24206-5-marcos.souza.org@gmail.com
State New
Headers show
Series
  • tests: do not fail if dm-thin is missing
Related show

Commit Message

Marcos Paulo de Souza Dec. 17, 2019, 8:31 p.m. UTC
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>
---
 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(-)

Comments

Qu Wenruo Dec. 18, 2019, 12:30 a.m. UTC | #1
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
>
Marcos Paulo de Souza Dec. 18, 2019, 12:44 a.m. UTC | #2
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
> > 
>
Qu Wenruo Dec. 18, 2019, 12:48 a.m. UTC | #3
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
>>>
>>
>
David Sterba Dec. 18, 2019, 4:03 p.m. UTC | #4
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.

Patch
diff mbox series

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