[PATCH] test: Fix filesystem tests always being skipped

[Andy Shevchenko]

On Thu, May 20, 2021 at 10:09:46PM +0300, Alper Nebi Yasak wrote:
> Commit 1ba21bb06b08 ("test: Don't unmount not (yet) mounted system")
> fixes an issue in the filesystem tests where the test setup may fail
> to mount an image and still attempt to unmount it. However, the commit
> unintentionally breaks the test setups in two ways.
> 
> The newly created unmounted filesystem images are being immediately
> deleted due to some cleanup steps being misplaced into finally blocks,
> which makes them always run instead of only on failures. The mount calls
> always fail since the images never exist, causing the tests to be always
> skipped. This patch moves these cleanup calls into the except blocks to
> fix this and makes the tests run again.
> 
> There are also unmount calls misplaced into finally blocks, making them
> run after the tests instead of before the tests. These unmount calls
> make the filesystem image file consistent with the changes made to it as
> part of the test setup, and this misplacement is making a number of
> tests fail unexpectedly.
> 
> The unmount calls must be run before the tests use the image, meaning
> before the yield call and not in the finally block. They must also be
> run as a cleanup step when the filesystem setup fails, so they can't be
> placed as the final call in the try blocks since they would be skipped
> on such failures. For these reasons, this patch places the unmount calls
> both in the except blocks and the else blocks of the final setup step.
> This makes the unexpectedly failing tests to succeed again.
> 
> Furthermore, this isolates the mount calls to their own try-except
> statement to avoid reintroducing the original issue of unmounting a
> not-mounted image while fixing the unmount misplacement.
> 
> After these fixes, running "make tests" with guestmount available results
> in two test failures not related to the mentioned commit. If the
> guestmount executables are unavailable, the mounts fallback to using
> sudo and result in no failures.
> 
> Fixes: 1ba21bb06b08 ("test: Don't unmount not (yet) mounted system")
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
> ---
> There is more discussion at Heinrich's revert-patch [1] of the mentioned
> commit. This patch is an alternative to the revert.
> 
> Andy: I don't think unmounting the failed-to-mount image was the cause
> of the sudo call you described in that commit, so I believe this would
> end up running sudo on your system. Wanted to warn in advance, but looks
> like your issue needs a different solution.

Thanks for doing this!

If you think this fix is a right thing to do, go ahead for it, I prefer it over
revert.

Acked-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com>

> [1] https://patchwork.ozlabs.org/project/uboot/patch/20210513114035.177293-1-xypron.glpk@gmx.de/
> 
>  test/py/tests/test_fs/conftest.py | 48 +++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
> index 50af9efcf768..410a675b9714 100644
> --- a/test/py/tests/test_fs/conftest.py
> +++ b/test/py/tests/test_fs/conftest.py
> @@ -278,14 +278,19 @@ def fs_obj_basic(request, u_boot_config):
>          check_call('mkdir -p %s' % mount_dir, shell=True)
>      except CalledProcessError as err:
>          pytest.skip('Preparing mount folder failed for filesystem: ' + fs_type + '. {}'.format(err))
> -        return
> -    finally:
>          call('rm -f %s' % fs_img, shell=True)
> +        return
>  
>      try:
>          # Mount the image so we can populate it.
>          mount_fs(fs_type, fs_img, mount_dir)
> +    except CalledProcessError as err:
> +        pytest.skip('Mounting to folder failed for filesystem: ' + fs_type + '. {}'.format(err))
> +        call('rmdir %s' % mount_dir, shell=True)
> +        call('rm -f %s' % fs_img, shell=True)
> +        return
>  
> +    try:
>          # Create a subdirectory.
>          check_call('mkdir %s/SUBDIR' % mount_dir, shell=True)
>  
> @@ -348,11 +353,12 @@ def fs_obj_basic(request, u_boot_config):
>  
>      except CalledProcessError as err:
>          pytest.skip('Setup failed for filesystem: ' + fs_type + '. {}'.format(err))
> +        umount_fs(mount_dir)
>          return
>      else:
> +        umount_fs(mount_dir)
>          yield [fs_ubtype, fs_img, md5val]
>      finally:
> -        umount_fs(mount_dir)
>          call('rmdir %s' % mount_dir, shell=True)
>          call('rm -f %s' % fs_img, shell=True)
>  
> @@ -394,14 +400,19 @@ def fs_obj_ext(request, u_boot_config):
>          check_call('mkdir -p %s' % mount_dir, shell=True)
>      except CalledProcessError as err:
>          pytest.skip('Preparing mount folder failed for filesystem: ' + fs_type + '. {}'.format(err))
> -        return
> -    finally:
>          call('rm -f %s' % fs_img, shell=True)
> +        return
>  
>      try:
>          # Mount the image so we can populate it.
>          mount_fs(fs_type, fs_img, mount_dir)
> +    except CalledProcessError as err:
> +        pytest.skip('Mounting to folder failed for filesystem: ' + fs_type + '. {}'.format(err))
> +        call('rmdir %s' % mount_dir, shell=True)
> +        call('rm -f %s' % fs_img, shell=True)
> +        return
>  
> +    try:
>          # Create a test directory
>          check_call('mkdir %s/dir1' % mount_dir, shell=True)
>  
> @@ -443,11 +454,12 @@ def fs_obj_ext(request, u_boot_config):
>          check_call('rm %s' % tmp_file, shell=True)
>      except CalledProcessError:
>          pytest.skip('Setup failed for filesystem: ' + fs_type)
> +        umount_fs(mount_dir)
>          return
>      else:
> +        umount_fs(mount_dir)
>          yield [fs_ubtype, fs_img, md5val]
>      finally:
> -        umount_fs(mount_dir)
>          call('rmdir %s' % mount_dir, shell=True)
>          call('rm -f %s' % fs_img, shell=True)
>  
> @@ -517,14 +529,19 @@ def fs_obj_unlink(request, u_boot_config):
>          check_call('mkdir -p %s' % mount_dir, shell=True)
>      except CalledProcessError as err:
>          pytest.skip('Preparing mount folder failed for filesystem: ' + fs_type + '. {}'.format(err))
> -        return
> -    finally:
>          call('rm -f %s' % fs_img, shell=True)
> +        return
>  
>      try:
>          # Mount the image so we can populate it.
>          mount_fs(fs_type, fs_img, mount_dir)
> +    except CalledProcessError as err:
> +        pytest.skip('Mounting to folder failed for filesystem: ' + fs_type + '. {}'.format(err))
> +        call('rmdir %s' % mount_dir, shell=True)
> +        call('rm -f %s' % fs_img, shell=True)
> +        return
>  
> +    try:
>          # Test Case 1 & 3
>          check_call('mkdir %s/dir1' % mount_dir, shell=True)
>          check_call('dd if=/dev/urandom of=%s/dir1/file1 bs=1K count=1'
> @@ -548,11 +565,12 @@ def fs_obj_unlink(request, u_boot_config):
>  
>      except CalledProcessError:
>          pytest.skip('Setup failed for filesystem: ' + fs_type)
> +        umount_fs(mount_dir)
>          return
>      else:
> +        umount_fs(mount_dir)
>          yield [fs_ubtype, fs_img]
>      finally:
> -        umount_fs(mount_dir)
>          call('rmdir %s' % mount_dir, shell=True)
>          call('rm -f %s' % fs_img, shell=True)
>  
> @@ -594,14 +612,19 @@ def fs_obj_symlink(request, u_boot_config):
>          check_call('mkdir -p %s' % mount_dir, shell=True)
>      except CalledProcessError as err:
>          pytest.skip('Preparing mount folder failed for filesystem: ' + fs_type + '. {}'.format(err))
> -        return
> -    finally:
>          call('rm -f %s' % fs_img, shell=True)
> +        return
>  
>      try:
>          # Mount the image so we can populate it.
>          mount_fs(fs_type, fs_img, mount_dir)
> +    except CalledProcessError as err:
> +        pytest.skip('Mounting to folder failed for filesystem: ' + fs_type + '. {}'.format(err))
> +        call('rmdir %s' % mount_dir, shell=True)
> +        call('rm -f %s' % fs_img, shell=True)
> +        return
>  
> +    try:
>          # Create a subdirectory.
>          check_call('mkdir %s/SUBDIR' % mount_dir, shell=True)
>  
> @@ -625,10 +648,11 @@ def fs_obj_symlink(request, u_boot_config):
>  
>      except CalledProcessError:
>          pytest.skip('Setup failed for filesystem: ' + fs_type)
> +        umount_fs(mount_dir)
>          return
>      else:
> +        umount_fs(mount_dir)
>          yield [fs_ubtype, fs_img, md5val]
>      finally:
> -        umount_fs(mount_dir)
>          call('rmdir %s' % mount_dir, shell=True)
>          call('rm -f %s' % fs_img, shell=True)
> -- 
> 2.31.1
> 

-- 
With Best Regards,
Andy Shevchenko