[PATCH] test: Fix filesystem tests always being skipped

* [PATCH] test: Fix filesystem tests always being skipped
@ 2021-05-20 19:09 Alper Nebi Yasak
  2021-05-24 19:16 ` Andy Shevchenko
  2021-05-27 11:42 ` Tom Rini
  0 siblings, 2 replies; 3+ messages in thread
From: Alper Nebi Yasak @ 2021-05-20 19:09 UTC (permalink / raw)
  To: u-boot

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@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.

[1] https://patchwork.ozlabs.org/project/uboot/patch/20210513114035.177293-1-xypron.glpk at 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

^ permalink raw reply related	[flat|nested] 3+ messages in thread