[PATCH v2 1/4] test: Include /sbin to the PATH when creating ext4 disk image
On some distributions the mkfs.ext4 is under /sbin and /sbin is not set for mere users. Include /sbin to the PATH when creating ext4 disk image, so that users won't get a scary traceback from Python.
Cc: Patrick Delaunay patrick.delaunay@foss.st.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- v2: used '/sbin' as is (Simon) test/py/tests/test_env.py | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py index 940279651da0..9bed2f48d77e 100644 --- a/test/py/tests/test_env.py +++ b/test/py/tests/test_env.py @@ -414,6 +414,8 @@ def mk_env_ext4(state_test_env): if os.path.exists(persistent): c.log.action('Disk image file ' + persistent + ' already exists') else: + # Some distributions do not add /sbin to the default PATH, where mkfs.ext4 lives + os.environ["PATH"] += os.pathsep + '/sbin' try: u_boot_utils.run_and_log(c, 'dd if=/dev/zero of=%s bs=1M count=16' % persistent) u_boot_utils.run_and_log(c, 'mkfs.ext4 %s' % persistent)
When run `ut dm [test name]` allow to use simple pattern to run all tests started with given prefix. For example, to run all ACPI test cases: ut dm acpi*
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- v2: rebased against dm/test-working branch (Simon) test/test-main.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/test/test-main.c b/test/test-main.c index e1b49e091ab6..8fcbc2361214 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -128,10 +128,17 @@ static bool ut_test_run_on_flattree(struct unit_test *test) static bool test_matches(const char *prefix, const char *test_name, const char *select_name) { + size_t len; + if (!select_name) return true;
- if (!strcmp(test_name, select_name)) + /* Allow glob expansion in the test name */ + len = select_name[strlen(select_name) - 1] == '*' ? strlen(select_name) : 0; + if (len-- == 1) + return true; + + if (!strncmp(test_name, select_name, len)) return true;
if (!prefix) { @@ -146,7 +153,7 @@ static bool test_matches(const char *prefix, const char *test_name, test_name += strlen(prefix); }
- if (!strcmp(test_name, select_name)) + if (!strncmp(test_name, select_name, len)) return true;
return false;
On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
When run `ut dm [test name]` allow to use simple pattern to run all tests started with given prefix. For example, to run all ACPI test cases: ut dm acpi*
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: rebased against dm/test-working branch (Simon)
Sadly that is deferred, but we can pick this patch up later.
test/test-main.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
When run `ut dm [test name]` allow to use simple pattern to run all tests started with given prefix. For example, to run all ACPI test cases: ut dm acpi*
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: rebased against dm/test-working branch (Simon)
Sadly that is deferred, but we can pick this patch up later.
test/test-main.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm, thanks!
It is easier to read the positive conditional.
While at it, convert hard coded length of "_test_" to strlen("_test_") which will be converted to a constant bu optimizing compiler.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- v2: new patch test/test-main.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/test/test-main.c b/test/test-main.c index 8fcbc2361214..344122074e12 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -141,16 +141,16 @@ static bool test_matches(const char *prefix, const char *test_name, if (!strncmp(test_name, select_name, len)) return true;
- if (!prefix) { + if (prefix) { + /* All tests have this prefix */ + if (!strncmp(test_name, prefix, strlen(prefix))) + test_name += strlen(prefix); + } else { const char *p = strstr(test_name, "_test_");
/* convert xxx_test_yyy to yyy, i.e. remove the suite name */ if (p) - test_name = p + 6; - } else { - /* All tests have this prefix */ - if (!strncmp(test_name, prefix, strlen(prefix))) - test_name += strlen(prefix); + test_name = p + strlen("_test_"); }
if (!strncmp(test_name, select_name, len))
On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
It is easier to read the positive conditional.
While at it, convert hard coded length of "_test_" to strlen("_test_") which will be converted to a constant bu optimizing compiler.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: new patch test/test-main.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
It is easier to read the positive conditional.
While at it, convert hard coded length of "_test_" to strlen("_test_") which will be converted to a constant bu optimizing compiler.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: new patch test/test-main.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm, thanks!
When test suite tries to create a file for a new filesystem test case and fails, the clean up of the exception tries to unmount the image, that has not yet been mounted. When it happens, the fuse_mounted global variable is set to False and inconveniently the test case tries to use sudo, so without this change the admin of the machine gets an (annoying) email:
Subject: *** SECURITY information for example.com ***
example.com : Feb 5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
and second run of the test cases on uncleaned build folder will ask for sudo which is not what expected.
Besides that there is a double unmount calls during successfully run test case.
All of these due to over engineered Python try-except clause and people didn't get it properly at all. The rule of thumb is that don't use more keywords than try-except in the exception handling code. Nevertheless, here we adjust code to be less intrusive to the initial logic behind that complex and unclear constructions in the test case, although it adds a lot of lines of the code, i.e. splits one exception handler to three, so on each step we know what cleanup shall perform.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- v2: new patch test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 22 deletions(-)
diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py index ec70e8c4ef3f..50af9efcf768 100644 --- a/test/py/tests/test_fs/conftest.py +++ b/test/py/tests/test_fs/conftest.py @@ -270,9 +270,20 @@ def fs_obj_basic(request, u_boot_config):
# 3GiB volume fs_img = mk_fs(u_boot_config, fs_type, 0xc0000000, '3GB') + except CalledProcessError as err: + pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err)) + return
- # Mount the image so we can populate it. + try: 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) + + try: + # Mount the image so we can populate it. mount_fs(fs_type, fs_img, mount_dir)
# Create a subdirectory. @@ -335,18 +346,15 @@ def fs_obj_basic(request, u_boot_config): % big_file, shell=True).decode() md5val.append(out.split()[0])
- umount_fs(mount_dir) except CalledProcessError as err: - pytest.skip('Setup failed for filesystem: ' + fs_type + \ - '. {}'.format(err)) + pytest.skip('Setup failed for filesystem: ' + fs_type + '. {}'.format(err)) return else: yield [fs_ubtype, fs_img, md5val] finally: umount_fs(mount_dir) call('rmdir %s' % mount_dir, shell=True) - if fs_img: - call('rm -f %s' % fs_img, shell=True) + call('rm -f %s' % fs_img, shell=True)
# # Fixture for extended fs test @@ -378,9 +386,20 @@ def fs_obj_ext(request, u_boot_config):
# 128MiB volume fs_img = mk_fs(u_boot_config, fs_type, 0x8000000, '128MB') + except CalledProcessError as err: + pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err)) + return
- # Mount the image so we can populate it. + try: 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) + + try: + # Mount the image so we can populate it. mount_fs(fs_type, fs_img, mount_dir)
# Create a test directory @@ -422,7 +441,6 @@ def fs_obj_ext(request, u_boot_config): md5val.append(out.split()[0])
check_call('rm %s' % tmp_file, shell=True) - umount_fs(mount_dir) except CalledProcessError: pytest.skip('Setup failed for filesystem: ' + fs_type) return @@ -431,8 +449,7 @@ def fs_obj_ext(request, u_boot_config): finally: umount_fs(mount_dir) call('rmdir %s' % mount_dir, shell=True) - if fs_img: - call('rm -f %s' % fs_img, shell=True) + call('rm -f %s' % fs_img, shell=True)
# # Fixture for mkdir test @@ -460,11 +477,10 @@ def fs_obj_mkdir(request, u_boot_config): fs_img = mk_fs(u_boot_config, fs_type, 0x8000000, '128MB') except: pytest.skip('Setup failed for filesystem: ' + fs_type) + return else: yield [fs_ubtype, fs_img] - finally: - if fs_img: - call('rm -f %s' % fs_img, shell=True) + call('rm -f %s' % fs_img, shell=True)
# # Fixture for unlink test @@ -493,9 +509,20 @@ def fs_obj_unlink(request, u_boot_config):
# 128MiB volume fs_img = mk_fs(u_boot_config, fs_type, 0x8000000, '128MB') + except CalledProcessError as err: + pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err)) + return
- # Mount the image so we can populate it. + try: 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) + + try: + # Mount the image so we can populate it. mount_fs(fs_type, fs_img, mount_dir)
# Test Case 1 & 3 @@ -519,7 +546,6 @@ def fs_obj_unlink(request, u_boot_config): check_call('dd if=/dev/urandom of=%s/dir5/file1 bs=1K count=1' % mount_dir, shell=True)
- umount_fs(mount_dir) except CalledProcessError: pytest.skip('Setup failed for filesystem: ' + fs_type) return @@ -528,8 +554,7 @@ def fs_obj_unlink(request, u_boot_config): finally: umount_fs(mount_dir) call('rmdir %s' % mount_dir, shell=True) - if fs_img: - call('rm -f %s' % fs_img, shell=True) + call('rm -f %s' % fs_img, shell=True)
# # Fixture for symlink fs test @@ -559,11 +584,22 @@ def fs_obj_symlink(request, u_boot_config):
try:
- # 3GiB volume + # 1GiB volume fs_img = mk_fs(u_boot_config, fs_type, 0x40000000, '1GB') + except CalledProcessError as err: + pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err)) + return
- # Mount the image so we can populate it. + try: 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) + + try: + # Mount the image so we can populate it. mount_fs(fs_type, fs_img, mount_dir)
# Create a subdirectory. @@ -587,7 +623,6 @@ def fs_obj_symlink(request, u_boot_config): % medium_file, shell=True).decode() md5val.extend([out.split()[0]])
- umount_fs(mount_dir) except CalledProcessError: pytest.skip('Setup failed for filesystem: ' + fs_type) return @@ -596,5 +631,4 @@ def fs_obj_symlink(request, u_boot_config): finally: umount_fs(mount_dir) call('rmdir %s' % mount_dir, shell=True) - if fs_img: - call('rm -f %s' % fs_img, shell=True) + call('rm -f %s' % fs_img, shell=True)
Hi Andy,
On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
When test suite tries to create a file for a new filesystem test case and fails, the clean up of the exception tries to unmount the image, that has not yet been mounted. When it happens, the fuse_mounted global variable is set to False and inconveniently the test case tries to use sudo, so without this change the admin of the machine gets an (annoying) email:
Subject: *** SECURITY information for example.com ***
example.com : Feb 5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
and second run of the test cases on uncleaned build folder will ask for sudo which is not what expected.
Besides that there is a double unmount calls during successfully run test case.
All of these due to over engineered Python try-except clause and people didn't get it properly at all. The rule of thumb is that don't use more keywords than try-except in the exception handling code. Nevertheless, here we adjust code to be less intrusive to the initial logic behind that complex and unclear constructions in the test case, although it adds a lot of lines of the code, i.e. splits one exception handler to three, so on each step we know what cleanup shall perform.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: new patch test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 22 deletions(-)
This looks OK to me, but there is a lot of duplication in the code, isn't there? Perhaps another forray?
Regards, Simon
On Thu, Feb 18, 2021 at 6:46 AM Simon Glass sjg@chromium.org wrote:
Hi Andy,
On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
When test suite tries to create a file for a new filesystem test case and fails, the clean up of the exception tries to unmount the image, that has not yet been mounted. When it happens, the fuse_mounted global variable is set to False and inconveniently the test case tries to use sudo, so without this change the admin of the machine gets an (annoying) email:
Subject: *** SECURITY information for example.com ***
example.com : Feb 5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
and second run of the test cases on uncleaned build folder will ask for sudo which is not what expected.
Besides that there is a double unmount calls during successfully run test case.
All of these due to over engineered Python try-except clause and people didn't get it properly at all. The rule of thumb is that don't use more keywords than try-except in the exception handling code. Nevertheless, here we adjust code to be less intrusive to the initial logic behind that complex and unclear constructions in the test case, although it adds a lot of lines of the code, i.e. splits one exception handler to three, so on each step we know what cleanup shall perform.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: new patch test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 22 deletions(-)
This looks OK to me, but there is a lot of duplication in the code, isn't there? Perhaps another forray?
Can we apply this fix as is and think about optimisations later, please? W/o this I'm really blocked from running tests against U-Boot.
On Thu, 18 Feb 2021 at 03:56, Andy Shevchenko andy.shevchenko@gmail.com wrote:
On Thu, Feb 18, 2021 at 6:46 AM Simon Glass sjg@chromium.org wrote:
Hi Andy,
On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
When test suite tries to create a file for a new filesystem test case and fails, the clean up of the exception tries to unmount the image, that has not yet been mounted. When it happens, the fuse_mounted global variable is set to False and inconveniently the test case tries to use sudo, so without this change the admin of the machine gets an (annoying) email:
Subject: *** SECURITY information for example.com ***
example.com : Feb 5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
and second run of the test cases on uncleaned build folder will ask for sudo which is not what expected.
Besides that there is a double unmount calls during successfully run test case.
All of these due to over engineered Python try-except clause and people didn't get it properly at all. The rule of thumb is that don't use more keywords than try-except in the exception handling code. Nevertheless, here we adjust code to be less intrusive to the initial logic behind that complex and unclear constructions in the test case, although it adds a lot of lines of the code, i.e. splits one exception handler to three, so on each step we know what cleanup shall perform.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: new patch test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 22 deletions(-)
This looks OK to me, but there is a lot of duplication in the code, isn't there? Perhaps another forray?
Can we apply this fix as is and think about optimisations later, please? W/o this I'm really blocked from running tests against U-Boot.
'make qcheck' bypasses this.
+Heinrich Schuchardt
Reviewed-by: Simon Glass sjg@chromium.org
On Thu, Feb 18, 2021 at 09:52:12PM -0700, Simon Glass wrote:
On Thu, 18 Feb 2021 at 03:56, Andy Shevchenko andy.shevchenko@gmail.com wrote:
On Thu, Feb 18, 2021 at 6:46 AM Simon Glass sjg@chromium.org wrote:
On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
When test suite tries to create a file for a new filesystem test case and fails, the clean up of the exception tries to unmount the image, that has not yet been mounted. When it happens, the fuse_mounted global variable is set to False and inconveniently the test case tries to use sudo, so without this change the admin of the machine gets an (annoying) email:
Subject: *** SECURITY information for example.com ***
example.com : Feb 5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
and second run of the test cases on uncleaned build folder will ask for sudo which is not what expected.
Besides that there is a double unmount calls during successfully run test case.
All of these due to over engineered Python try-except clause and people didn't get it properly at all. The rule of thumb is that don't use more keywords than try-except in the exception handling code. Nevertheless, here we adjust code to be less intrusive to the initial logic behind that complex and unclear constructions in the test case, although it adds a lot of lines of the code, i.e. splits one exception handler to three, so on each step we know what cleanup shall perform.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: new patch test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 22 deletions(-)
This looks OK to me, but there is a lot of duplication in the code, isn't there? Perhaps another forray?
Can we apply this fix as is and think about optimisations later, please? W/o this I'm really blocked from running tests against U-Boot.
'make qcheck' bypasses this.
+Heinrich Schuchardt
Reviewed-by: Simon Glass sjg@chromium.org
Thanks!
Tom, I don't see this being applied. Can we actually get it in?
On Tue, Mar 30, 2021 at 10:41:15PM +0300, Andy Shevchenko wrote:
On Thu, Feb 18, 2021 at 09:52:12PM -0700, Simon Glass wrote:
On Thu, 18 Feb 2021 at 03:56, Andy Shevchenko andy.shevchenko@gmail.com wrote:
On Thu, Feb 18, 2021 at 6:46 AM Simon Glass sjg@chromium.org wrote:
On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
When test suite tries to create a file for a new filesystem test case and fails, the clean up of the exception tries to unmount the image, that has not yet been mounted. When it happens, the fuse_mounted global variable is set to False and inconveniently the test case tries to use sudo, so without this change the admin of the machine gets an (annoying) email:
Subject: *** SECURITY information for example.com ***
example.com : Feb 5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
and second run of the test cases on uncleaned build folder will ask for sudo which is not what expected.
Besides that there is a double unmount calls during successfully run test case.
All of these due to over engineered Python try-except clause and people didn't get it properly at all. The rule of thumb is that don't use more keywords than try-except in the exception handling code. Nevertheless, here we adjust code to be less intrusive to the initial logic behind that complex and unclear constructions in the test case, although it adds a lot of lines of the code, i.e. splits one exception handler to three, so on each step we know what cleanup shall perform.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: new patch test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 22 deletions(-)
This looks OK to me, but there is a lot of duplication in the code, isn't there? Perhaps another forray?
Can we apply this fix as is and think about optimisations later, please? W/o this I'm really blocked from running tests against U-Boot.
'make qcheck' bypasses this.
+Heinrich Schuchardt
Reviewed-by: Simon Glass sjg@chromium.org
Thanks!
Tom, I don't see this being applied. Can we actually get it in?
Does this apply cleanly to master? I thought only 1/4 did so I applied that.
On Tuesday, March 30, 2021, Tom Rini trini@konsulko.com wrote:
On Tue, Mar 30, 2021 at 10:41:15PM +0300, Andy Shevchenko wrote:
On Thu, Feb 18, 2021 at 09:52:12PM -0700, Simon Glass wrote:
On Thu, 18 Feb 2021 at 03:56, Andy Shevchenko <
andy.shevchenko@gmail.com> wrote:
On Thu, Feb 18, 2021 at 6:46 AM Simon Glass sjg@chromium.org
wrote:
On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
When test suite tries to create a file for a new filesystem test
case and fails,
the clean up of the exception tries to unmount the image, that
has not yet been
mounted. When it happens, the fuse_mounted global variable is
set to False and
inconveniently the test case tries to use sudo, so without this
change the
admin of the machine gets an (annoying) email:
Subject: *** SECURITY information for example.com ***
example.com : Feb 5 19:43:47 : ... COMMAND=/bin/umount
.../build-sandbox/persistent-data/mnt
and second run of the test cases on uncleaned build folder will
ask for sudo
which is not what expected.
Besides that there is a double unmount calls during successfully
run test case.
All of these due to over engineered Python try-except clause and
people didn't
get it properly at all. The rule of thumb is that don't use more
keywords than
try-except in the exception handling code. Nevertheless, here we
adjust code
to be less intrusive to the initial logic behind that complex
and unclear
constructions in the test case, although it adds a lot of lines
of the code,
i.e. splits one exception handler to three, so on each step we
know what
cleanup shall perform.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.
intel.com>
v2: new patch test/py/tests/test_fs/conftest.py | 78
++++++++++++++++++++++---------
1 file changed, 56 insertions(+), 22 deletions(-)
This looks OK to me, but there is a lot of duplication in the code, isn't there? Perhaps another forray?
Can we apply this fix as is and think about optimisations later,
please?
W/o this I'm really blocked from running tests against U-Boot.
'make qcheck' bypasses this.
+Heinrich Schuchardt
Reviewed-by: Simon Glass sjg@chromium.org
Thanks!
Tom, I don't see this being applied. Can we actually get it in?
Does this apply cleanly to master? I thought only 1/4 did so I applied that.
Patches 2&3 are against Simon’s tree, patches 1&4 are against main branch
-- Tom
On Thu, Feb 11, 2021 at 04:40:12PM +0200, Andy Shevchenko wrote:
When test suite tries to create a file for a new filesystem test case and fails, the clean up of the exception tries to unmount the image, that has not yet been mounted. When it happens, the fuse_mounted global variable is set to False and inconveniently the test case tries to use sudo, so without this change the admin of the machine gets an (annoying) email:
Subject: *** SECURITY information for example.com ***
example.com : Feb 5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
and second run of the test cases on uncleaned build folder will ask for sudo which is not what expected.
Besides that there is a double unmount calls during successfully run test case.
All of these due to over engineered Python try-except clause and people didn't get it properly at all. The rule of thumb is that don't use more keywords than try-except in the exception handling code. Nevertheless, here we adjust code to be less intrusive to the initial logic behind that complex and unclear constructions in the test case, although it adds a lot of lines of the code, i.e. splits one exception handler to three, so on each step we know what cleanup shall perform.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
On 2/11/21 3:40 PM, Andy Shevchenko wrote:
When test suite tries to create a file for a new filesystem test case and fails, the clean up of the exception tries to unmount the image, that has not yet been mounted. When it happens, the fuse_mounted global variable is set to False and inconveniently the test case tries to use sudo, so without this change the admin of the machine gets an (annoying) email:
Subject: *** SECURITY information for example.com ***
example.com : Feb 5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
and second run of the test cases on uncleaned build folder will ask for sudo which is not what expected.
Besides that there is a double unmount calls during successfully run test case.
All of these due to over engineered Python try-except clause and people didn't get it properly at all. The rule of thumb is that don't use more keywords than try-except in the exception handling code. Nevertheless, here we adjust code to be less intrusive to the initial logic behind that complex and unclear constructions in the test case, although it adds a lot of lines of the code, i.e. splits one exception handler to three, so on each step we know what cleanup shall perform.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
Dear Andy,
with this merged patch the following tests are not executed anymore locally:
test/py/tests/test_fs/test_basic.py test/py/tests/test_fs/test_ext.py
SKIPPED [13] test/py/tests/test_fs/conftest.py:350: Setup failed for filesystem: ext4. Command 'guestmount -a build-sandbox/persistent-data/3GB.ext4.img -m /dev/sda build-sandbox/persistent-data/mnt' returned non-zero exit status 1.
Please, revert the patch or fix it.
Best regards
Heinrich
Reviewed-by: Simon Glass sjg@chromium.org
v2: new patch test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 22 deletions(-)
diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py index ec70e8c4ef3f..50af9efcf768 100644 --- a/test/py/tests/test_fs/conftest.py +++ b/test/py/tests/test_fs/conftest.py @@ -270,9 +270,20 @@ def fs_obj_basic(request, u_boot_config):
# 3GiB volume fs_img = mk_fs(u_boot_config, fs_type, 0xc0000000, '3GB')
- except CalledProcessError as err:
pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err))return
# Mount the image so we can populate it.
try: 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))returnfinally:
call('rm -f %s' % fs_img, shell=True)try:
# Mount the image so we can populate it. mount_fs(fs_type, fs_img, mount_dir) # Create a subdirectory.@@ -335,18 +346,15 @@ def fs_obj_basic(request, u_boot_config): % big_file, shell=True).decode() md5val.append(out.split()[0])
umount_fs(mount_dir) except CalledProcessError as err:pytest.skip('Setup failed for filesystem: ' + fs_type + \'. {}'.format(err))
pytest.skip('Setup failed for filesystem: ' + fs_type + '. {}'.format(err)) return else: yield [fs_ubtype, fs_img, md5val] finally: umount_fs(mount_dir) call('rmdir %s' % mount_dir, shell=True)
if fs_img:call('rm -f %s' % fs_img, shell=True)
call('rm -f %s' % fs_img, shell=True)# # Fixture for extended fs test
@@ -378,9 +386,20 @@ def fs_obj_ext(request, u_boot_config):
# 128MiB volume fs_img = mk_fs(u_boot_config, fs_type, 0x8000000, '128MB')
- except CalledProcessError as err:
pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err))return
# Mount the image so we can populate it.
try: 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))returnfinally:
call('rm -f %s' % fs_img, shell=True)try:
# Mount the image so we can populate it. mount_fs(fs_type, fs_img, mount_dir) # Create a test directory@@ -422,7 +441,6 @@ def fs_obj_ext(request, u_boot_config): md5val.append(out.split()[0])
check_call('rm %s' % tmp_file, shell=True)
umount_fs(mount_dir) except CalledProcessError: pytest.skip('Setup failed for filesystem: ' + fs_type) return@@ -431,8 +449,7 @@ def fs_obj_ext(request, u_boot_config): finally: umount_fs(mount_dir) call('rmdir %s' % mount_dir, shell=True)
if fs_img:call('rm -f %s' % fs_img, shell=True)
call('rm -f %s' % fs_img, shell=True)# # Fixture for mkdir test
@@ -460,11 +477,10 @@ def fs_obj_mkdir(request, u_boot_config): fs_img = mk_fs(u_boot_config, fs_type, 0x8000000, '128MB') except: pytest.skip('Setup failed for filesystem: ' + fs_type)
return else: yield [fs_ubtype, fs_img]
- finally:
if fs_img:call('rm -f %s' % fs_img, shell=True)
call('rm -f %s' % fs_img, shell=True)
# # Fixture for unlink test
@@ -493,9 +509,20 @@ def fs_obj_unlink(request, u_boot_config):
# 128MiB volume fs_img = mk_fs(u_boot_config, fs_type, 0x8000000, '128MB')
- except CalledProcessError as err:
pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err))return
# Mount the image so we can populate it.
try: 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))returnfinally:
call('rm -f %s' % fs_img, shell=True)try:
# Mount the image so we can populate it. mount_fs(fs_type, fs_img, mount_dir) # Test Case 1 & 3@@ -519,7 +546,6 @@ def fs_obj_unlink(request, u_boot_config): check_call('dd if=/dev/urandom of=%s/dir5/file1 bs=1K count=1' % mount_dir, shell=True)
umount_fs(mount_dir) except CalledProcessError: pytest.skip('Setup failed for filesystem: ' + fs_type) return@@ -528,8 +554,7 @@ def fs_obj_unlink(request, u_boot_config): finally: umount_fs(mount_dir) call('rmdir %s' % mount_dir, shell=True)
if fs_img:call('rm -f %s' % fs_img, shell=True)
call('rm -f %s' % fs_img, shell=True)# # Fixture for symlink fs test
@@ -559,11 +584,22 @@ def fs_obj_symlink(request, u_boot_config):
try:
# 3GiB volume
# 1GiB volume fs_img = mk_fs(u_boot_config, fs_type, 0x40000000, '1GB')- except CalledProcessError as err:
pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err))return
# Mount the image so we can populate it.
try: 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))returnfinally:
call('rm -f %s' % fs_img, shell=True)try:
# Mount the image so we can populate it. mount_fs(fs_type, fs_img, mount_dir) # Create a subdirectory.@@ -587,7 +623,6 @@ def fs_obj_symlink(request, u_boot_config): % medium_file, shell=True).decode() md5val.extend([out.split()[0]])
umount_fs(mount_dir) except CalledProcessError: pytest.skip('Setup failed for filesystem: ' + fs_type) return@@ -596,5 +631,4 @@ def fs_obj_symlink(request, u_boot_config): finally: umount_fs(mount_dir) call('rmdir %s' % mount_dir, shell=True)
if fs_img:call('rm -f %s' % fs_img, shell=True)
call('rm -f %s' % fs_img, shell=True)
On Thu, May 13, 2021 at 2:32 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/11/21 3:40 PM, Andy Shevchenko wrote:
When test suite tries to create a file for a new filesystem test case and fails, the clean up of the exception tries to unmount the image, that has not yet been mounted. When it happens, the fuse_mounted global variable is set to False and inconveniently the test case tries to use sudo, so without this change the admin of the machine gets an (annoying) email:
Subject: *** SECURITY information for example.com ***
example.com : Feb 5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
and second run of the test cases on uncleaned build folder will ask for sudo which is not what expected.
Besides that there is a double unmount calls during successfully run test case.
All of these due to over engineered Python try-except clause and people didn't get it properly at all. The rule of thumb is that don't use more keywords than try-except in the exception handling code. Nevertheless, here we adjust code to be less intrusive to the initial logic behind that complex and unclear constructions in the test case, although it adds a lot of lines of the code, i.e. splits one exception handler to three, so on each step we know what cleanup shall perform.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
Dear Andy,
with this merged patch the following tests are not executed anymore locally:
test/py/tests/test_fs/test_basic.py test/py/tests/test_fs/test_ext.py
SKIPPED [13] test/py/tests/test_fs/conftest.py:350: Setup failed for filesystem: ext4. Command 'guestmount -a build-sandbox/persistent-data/3GB.ext4.img -m /dev/sda build-sandbox/persistent-data/mnt' returned non-zero exit status 1.
Please, revert the patch or fix it.
Thanks for the report, let's investigate it. And for the consistency let's continue this under the revert thread,
On Thu, Feb 11, 2021 at 04:40:09PM +0200, Andy Shevchenko wrote:
On some distributions the mkfs.ext4 is under /sbin and /sbin is not set for mere users. Include /sbin to the PATH when creating ext4 disk image, so that users won't get a scary traceback from Python.
Note, patches 1 and 4 may be applied to U-Boot sources directly (while patches 2 and 3 are based on top of dm/test-working). Moreover, patch 4 fixes a quite annoying fix, without which I have got reprimanded by an admin of our build system.
Please, consider to apply patch 4 ASAP.
Cc: Patrick Delaunay patrick.delaunay@foss.st.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: used '/sbin' as is (Simon) test/py/tests/test_env.py | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py index 940279651da0..9bed2f48d77e 100644 --- a/test/py/tests/test_env.py +++ b/test/py/tests/test_env.py @@ -414,6 +414,8 @@ def mk_env_ext4(state_test_env): if os.path.exists(persistent): c.log.action('Disk image file ' + persistent + ' already exists') else:
# Some distributions do not add /sbin to the default PATH, where mkfs.ext4 livesos.environ["PATH"] += os.pathsep + '/sbin' try: u_boot_utils.run_and_log(c, 'dd if=/dev/zero of=%s bs=1M count=16' % persistent) u_boot_utils.run_and_log(c, 'mkfs.ext4 %s' % persistent)-- 2.30.0
On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
On some distributions the mkfs.ext4 is under /sbin and /sbin is not set for mere users. Include /sbin to the PATH when creating ext4 disk image, so that users won't get a scary traceback from Python.
Cc: Patrick Delaunay patrick.delaunay@foss.st.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: used '/sbin' as is (Simon) test/py/tests/test_env.py | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
On Thu, Feb 11, 2021 at 04:40:09PM +0200, Andy Shevchenko wrote:
On some distributions the mkfs.ext4 is under /sbin and /sbin is not set for mere users. Include /sbin to the PATH when creating ext4 disk image, so that users won't get a scary traceback from Python.
It has been a long. Is it any news about this series? Can it be applied (at least patches that are fine against U-Boot current master)?
Cc: Patrick Delaunay patrick.delaunay@foss.st.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
v2: used '/sbin' as is (Simon) test/py/tests/test_env.py | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py index 940279651da0..9bed2f48d77e 100644 --- a/test/py/tests/test_env.py +++ b/test/py/tests/test_env.py @@ -414,6 +414,8 @@ def mk_env_ext4(state_test_env): if os.path.exists(persistent): c.log.action('Disk image file ' + persistent + ' already exists') else:
# Some distributions do not add /sbin to the default PATH, where mkfs.ext4 livesos.environ["PATH"] += os.pathsep + '/sbin' try: u_boot_utils.run_and_log(c, 'dd if=/dev/zero of=%s bs=1M count=16' % persistent) u_boot_utils.run_and_log(c, 'mkfs.ext4 %s' % persistent)-- 2.30.0
On Thu, Feb 11, 2021 at 04:40:09PM +0200, Andy Shevchenko wrote:
On some distributions the mkfs.ext4 is under /sbin and /sbin is not set for mere users. Include /sbin to the PATH when creating ext4 disk image, so that users won't get a scary traceback from Python.
Cc: Patrick Delaunay patrick.delaunay@foss.st.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (5)
-
Andy Shevchenko -
Andy Shevchenko -
Heinrich Schuchardt -
Simon Glass -
Tom Rini