[PATCH 0/3] test/py: Rewrite SquashFS commands test suite

[PATCH 0/3] test/py: Rewrite SquashFS commands test suite

[Joao Marcos Costa]

Hello,

This patch series fixes the following issues:
- poor strategy to check if files were properly loaded
- wrong quoting style for strings
- tests failing at the second run because of a wrong clean-up strategy

Finally, it improves:
- code overall documentation level, with more comments and better
  naming for functions and variables
- code readability by adding more helper functions
- completeness: more test cases were added for both sqfsls and sqfsload
  commands

The sqfsload new test suite may fail when testing images with fragmented
files if the patch I previously sent (fs/squashfs: fix reading of
fragmented files) is not applied, so this patch series depends on it.

Best regards,
Joao

Joao Marcos Costa (3):
  test/py: rewrite common tools for SquashFS tests
  test/py: rewrite sqfsload command test suite
  test/py: rewrite sqfsls command test suite

 .../test_fs/test_squashfs/sqfs_common.py      | 198 ++++++++++++------
 .../test_fs/test_squashfs/test_sqfs_load.py   |  99 ++++++---
 .../test_fs/test_squashfs/test_sqfs_ls.py     |  80 +++++--
 3 files changed, 264 insertions(+), 113 deletions(-)

-- 
2.25.1

[PATCH 1/3] test/py: rewrite common tools for SquashFS tests

[Joao Marcos Costa]

Remove the previous OOP approach, which was confusing and incomplete.
Add more test cases by making SquashFS images with various options,
concerning file fragmentation and its compression. Add comments to
properly document the code.

Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
---
 .../test_fs/test_squashfs/sqfs_common.py      | 198 ++++++++++++------
 1 file changed, 133 insertions(+), 65 deletions(-)

diff --git a/test/py/tests/test_fs/test_squashfs/sqfs_common.py b/test/py/tests/test_fs/test_squashfs/sqfs_common.py
index c96f92c1d8..81a378a9f9 100644
--- a/test/py/tests/test_fs/test_squashfs/sqfs_common.py
+++ b/test/py/tests/test_fs/test_squashfs/sqfs_common.py
@@ -1,76 +1,144 @@
 # SPDX-License-Identifier: GPL-2.0
-# Copyright (C) 2020 Bootlin
+# Copyright (C) 2021 Bootlin
 # Author: Joao Marcos Costa <joaomarcos.costa@bootlin.com>
 
 import os
-import random
-import string
+import shutil
 import subprocess
 
-def sqfs_get_random_letters(size):
-    letters = []
-    for i in range(0, size):
-            letters.append(random.choice(string.ascii_letters))
+"""
+standard test images table: Each table item is a key:value pair representing the
+output image name and its respective mksquashfs options. This table should be
+modified only when adding support for new compression algorithms.
+The 'default' case takes no options but the input and output names, so it must
+be assigned with an empty string.
+"""
+standard_table = {
+        'default' : '',
+        'lzo_comp_frag' : '',
+        'lzo_frag' : '',
+        'lzo_no_frag' : '',
+        'zstd_comp_frag' : '',
+        'zstd_frag' : '',
+        'zstd_no_frag' : '',
+        'gzip_comp_frag' : '',
+        'gzip_frag' : '',
+        'gzip_no_frag' : ''
+}
 
-    return ''.join(letters)
+"""
+extra_table: Set this table's keys and values if you want to make squashfs images with
+your own customized options.
+"""
+extra_table = {}
 
-def sqfs_generate_file(path, size):
-    content = sqfs_get_random_letters(size)
-    file = open(path, "w")
+# path to source directory used to make squashfs test images
+sqfs_src_dir = 'sqfs_src_dir'
+
+"""
+Combines fragmentation and compression options into a list of strings.
+opts_list's firts item is an empty string as standard_table's first item is
+the 'default' case.
+"""
+def get_opts_list():
+    # supported compression options only
+    comp_opts = ['-comp lzo', '-comp zstd', '-comp gzip']
+    # file fragmentation options
+    frag_opts = ['-always-use-fragments', '-always-use-fragments -noF', '-no-fragments']
+
+    opts_list = [' ']
+    for c in comp_opts:
+        for f in frag_opts:
+            opts_list.append(' '.join([c, f]))
+
+    return opts_list
+
+def init_standard_table():
+    opts_list = get_opts_list()
+
+    for key, value in zip(standard_table.keys(), opts_list):
+            standard_table[key] = value
+
+def generate_file(file_name, file_size):
+    # file is filled with file_size * 'x'
+    content = ''
+    for i in range(file_size):
+        content += 'x'
+
+    file = open(file_name, 'w')
     file.write(content)
     file.close()
 
-class Compression:
-    def __init__(self, name, files, sizes, block_size = 4096):
-        self.name = name
-        self.files = files
-        self.sizes = sizes
-        self.mksquashfs_opts = " -b " + str(block_size) + " -comp " + self.name
-
-    def add_opt(self, opt):
-        self.mksquashfs_opts += " " + opt
-
-    def gen_image(self, build_dir):
-        src = os.path.join(build_dir, "sqfs_src/")
-        os.mkdir(src)
-        for (f, s) in zip(self.files, self.sizes):
-            sqfs_generate_file(src + f, s)
-
-        # the symbolic link always targets the first file
-        os.symlink(self.files[0], src + "sym")
-
-        sqfs_img = os.path.join(build_dir, "sqfs-" + self.name)
-        i_o = src + " " + sqfs_img
-        opts = self.mksquashfs_opts
-        try:
-            subprocess.run(["mksquashfs " + i_o + opts], shell = True, check = True)
-        except:
-            print("mksquashfs error. Compression type: " + self.name)
-            raise RuntimeError
-
-    def clean_source(self, build_dir):
-        src = os.path.join(build_dir, "sqfs_src/")
-        for f in self.files:
-            os.remove(src + f)
-        os.remove(src + "sym")
-        os.rmdir(src)
-
-    def cleanup(self, build_dir):
-        self.clean_source(build_dir)
-        sqfs_img = os.path.join(build_dir, "sqfs-" + self.name)
-        os.remove(sqfs_img)
-
-files = ["blks_only", "blks_frag", "frag_only"]
-sizes = [4096, 5100, 100]
-gzip = Compression("gzip", files, sizes)
-zstd = Compression("zstd", files, sizes)
-lzo = Compression("lzo", files, sizes)
-
-# use fragment blocks for files larger than block_size
-gzip.add_opt("-always-use-fragments")
-zstd.add_opt("-always-use-fragments")
-
-# avoid fragments if lzo is used
-lzo.add_opt("-no-fragments")
-
-comp_opts = [gzip, zstd, lzo]
+"""
+sqfs_src_dir
+├── empty-dir
+├── f1000
+├── f4096
+├── f5096
+├── subdir
+│   └── subdir-file
+└── sym -> subdir
+
+3 directories, 4 files
+"""
+def generate_sqfs_src_dir(build_dir):
+    path = os.path.join(build_dir, sqfs_src_dir)
+    print(path)
+    #path = build_dir + '/' + sqfs_src_dir
+    # make root directory
+    os.makedirs(path)
+
+    # 4096: minimum block size
+    file_name = 'f4096'
+    generate_file(os.path.join(path, file_name), 4096)
+
+    # 5096: minimum block size + 1000 chars (fragment)
+    file_name = 'f5096'
+    generate_file(os.path.join(path, file_name), 5096)
+
+    # 1000: less than minimum block size (fragment only)
+    file_name = 'f1000'
+    generate_file(os.path.join(path, file_name), 1000)
+
+    # sub-directory with a single file inside
+    subdir_path = os.path.join(path, 'subdir')
+    os.makedirs(subdir_path)
+    generate_file(os.path.join(subdir_path, 'subdir-file'), 100)
+
+    # symlink (target: sub-directory)
+    os.symlink('subdir', os.path.join(path, 'sym'))
+
+    # empty directory
+    os.makedirs(os.path.join(path, 'empty-dir'))
+
+def mksquashfs(args):
+    subprocess.run(['mksquashfs ' + args], shell = True, check = True,
+            stdout = subprocess.DEVNULL)
+
+def make_all_images(build_dir):
+
+    init_standard_table()
+    input_path = os.path.join(build_dir, sqfs_src_dir)
+
+    # make squashfs images according to standard_table
+    for out, opts in zip(standard_table.keys(), standard_table.values()):
+        output_path = os.path.join(build_dir, out)
+        mksquashfs(' '.join([input_path, output_path, opts]))
+
+    # make squashfs images according to extra_table
+    for out, opts in zip(extra_table.keys(), extra_table.values()):
+        output_path = os.path.join(build_dir, out)
+        mksquashfs(' '.join([input_path, output_path, opts]))
+
+def clean_all_images(build_dir):
+    for image_name in standard_table.keys():
+        image_path = os.path.join(build_dir, image_name)
+        os.remove(image_path)
+
+    for image_name in extra_table.keys():
+        image_path = os.path.join(build_dir, image_name)
+        os.remove(image_path)
+
+def clean_sqfs_src_dir(build_dir):
+    path = os.path.join(build_dir, sqfs_src_dir)
+    shutil.rmtree(path)
-- 
2.25.1

[PATCH 2/3] test/py: rewrite sqfsload command test suite

[Joao Marcos Costa]

The previous strategy to know if a file was correctly loaded was to
check for how many bytes were read and compare it against the file's
original size. Since this is not a good solution, replace it by
comparing the checksum of the loaded bytes against the original file's
checksum. Add more test cases: files at a sub-directory and non-existent
file.

Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
---
 .../test_fs/test_squashfs/test_sqfs_load.py   | 99 +++++++++++++------
 1 file changed, 69 insertions(+), 30 deletions(-)

diff --git a/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py b/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py
index 9e90062384..0b416eb4c3 100644
--- a/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py
+++ b/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py
@@ -1,11 +1,62 @@
 # SPDX-License-Identifier: GPL-2.0
-# Copyright (C) 2020 Bootlin
+# Copyright (C) 2021 Bootlin
 # Author: Joao Marcos Costa <joaomarcos.costa@bootlin.com>
 
 import os
 import pytest
 from sqfs_common import *
 
+@pytest.mark.requiredtool('md5sum')
+def original_md5sum(path):
+    out = subprocess.run(['md5sum ' + path], shell = True, check = True,
+            capture_output = True, text = True)
+    checksum = out.stdout.split()[0]
+
+    return checksum
+
+def uboot_md5sum(u_boot_console, address, count):
+    out = u_boot_console.run_command('md5sum {} {}'.format(address, count))
+    checksum = out.split()[-1]
+
+    return checksum
+
+# loads files and asserts checksums
+def sqfs_load_files(u_boot_console, files, sizes, address):
+    build_dir = u_boot_console.config.build_dir
+    for (f, size) in zip(files, sizes):
+        out = u_boot_console.run_command('sqfsload host 0 {} {}'.format(address, f))
+
+        # check if the right amount of bytes was read
+        assert size in out
+
+        # compare original file's checksum against u-boot's
+        u_boot_checksum = uboot_md5sum(u_boot_console, address, hex(int(size)))
+        original_checksum = original_md5sum(os.path.join(build_dir, sqfs_src_dir + '/' + f))
+        assert u_boot_checksum == original_checksum
+
+def sqfs_load_files_at_root(u_boot_console):
+    files = ['f4096', 'f5096', 'f1000']
+    sizes = ['4096', '5096', '1000']
+    address = '$kernel_addr_r'
+    sqfs_load_files(u_boot_console, files, sizes, address)
+
+def sqfs_load_files_at_subdir(u_boot_console):
+    files = ['subdir/subdir-file']
+    sizes = ['100']
+    address = '$kernel_addr_r'
+    sqfs_load_files(u_boot_console, files, sizes, address)
+
+def sqfs_load_non_existent_file(u_boot_console):
+    address = '$kernel_addr_r'
+    f = 'non-existent'
+    out = u_boot_console.run_command('sqfsload host 0 {} {}'.format(address, f))
+    assert 'Failed to load' in out
+
+def sqfs_run_all_load_tests(u_boot_console):
+    sqfs_load_files_at_root(u_boot_console)
+    sqfs_load_files_at_subdir(u_boot_console)
+    sqfs_load_non_existent_file(u_boot_console)
+
 @pytest.mark.boardspec('sandbox')
 @pytest.mark.buildconfigspec('cmd_fs_generic')
 @pytest.mark.buildconfigspec('cmd_squashfs')
@@ -13,34 +64,22 @@ from sqfs_common import *
 @pytest.mark.requiredtool('mksquashfs')
 def test_sqfs_load(u_boot_console):
     build_dir = u_boot_console.config.build_dir
-    command = "sqfsload host 0 $kernel_addr_r "
 
-    for opt in comp_opts:
-        # generate and load the squashfs image
+    # setup test environment
+    generate_sqfs_src_dir(build_dir)
+    make_all_images(build_dir)
+
+    # run all tests for each image
+    for image in standard_table.keys():
         try:
-            opt.gen_image(build_dir)
-        except RuntimeError:
-            opt.clean_source(build_dir)
-            # skip unsupported compression types
-            continue
-
-        path = os.path.join(build_dir, "sqfs-" + opt.name)
-        output = u_boot_console.run_command("host bind 0 " + path)
-
-        output = u_boot_console.run_command(command + "xxx")
-        assert "File not found." in output
-
-        for (f, s) in zip(opt.files, opt.sizes):
-            try:
-                output = u_boot_console.run_command(command + f)
-                assert str(s) in output
-            except:
-                assert False
-                opt.cleanup(build_dir)
-
-        # test symbolic link
-        output = u_boot_console.run_command(command + "sym")
-        assert str(opt.sizes[0]) in output
-
-        # remove generated files
-        opt.cleanup(build_dir)
+            image_path = os.path.join(build_dir, image)
+            u_boot_console.run_command('host bind 0 {}'.format(image_path))
+            sqfs_run_all_load_tests(u_boot_console)
+        except:
+            clean_all_images(build_dir)
+            clean_sqfs_src_dir(build_dir)
+            raise AssertionError
+
+    # clean test environment
+    clean_all_images(build_dir)
+    clean_sqfs_src_dir(build_dir)
-- 
2.25.1

[PATCH 3/3] test/py: rewrite sqfsls command test suite

[Joao Marcos Costa]

Add more details to test cases by comparing each expected line with the
command's output. Add new test cases:
- sqfsls at an empty directory
- sqfsls at a sub-directory

Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
---
 .../test_fs/test_squashfs/test_sqfs_ls.py     | 80 ++++++++++++++-----
 1 file changed, 62 insertions(+), 18 deletions(-)

diff --git a/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py b/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py
index a0dca2e2fc..2895aefc3b 100644
--- a/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py
+++ b/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py
@@ -6,6 +6,52 @@ import os
 import pytest
 from sqfs_common import *
 
+def sqfs_ls_at_root(u_boot_console):
+    # first and most basic test is to check if these two give the same output
+    no_slash = u_boot_console.run_command('sqfsls host 0')
+    slash = u_boot_console.run_command('sqfsls host 0 /')
+    assert no_slash == slash
+
+    expected_lines = ['empty-dir/', '1000   f1000', '4096   f4096', '5096   f5096',
+            'subdir/', '<SYM>   sym', '4 file(s), 2 dir(s)']
+
+    output = u_boot_console.run_command('sqfsls host 0')
+    for line in expected_lines:
+        assert line in output
+
+def sqfs_ls_at_empty_dir(u_boot_console):
+    assert u_boot_console.run_command('sqfsls host 0 empty-dir') == 'Empty directory.'
+
+def sqfs_ls_at_subdir(u_boot_console):
+    expected_lines = ['100   subdir-file', '1 file(s), 0 dir(s)']
+    output = u_boot_console.run_command('sqfsls host 0 subdir')
+    for line in expected_lines:
+        assert line in output
+
+def sqfs_ls_at_symlink(u_boot_console):
+    # since sym -> subdir, the following outputs must be equal
+    output = u_boot_console.run_command('sqfsls host 0 sym')
+    output_subdir= u_boot_console.run_command('sqfsls host 0 subdir')
+    assert output == output_subdir
+
+    expected_lines = ['100   subdir-file', '1 file(s), 0 dir(s)']
+    for line in expected_lines:
+        assert line in output
+
+# output should be the same for non-existent directories and files
+def sqfs_ls_at_non_existent_dir(u_boot_console):
+    out_non_existent = u_boot_console.run_command('sqfsls host 0 fff')
+    out_not_dir = u_boot_console.run_command('sqfsls host 0 f1000')
+    assert out_non_existent == out_not_dir
+    assert '** Cannot find directory. **' in out_non_existent
+
+def sqfs_run_all_ls_tests(u_boot_console):
+    sqfs_ls_at_root(u_boot_console)
+    sqfs_ls_at_empty_dir(u_boot_console)
+    sqfs_ls_at_subdir(u_boot_console)
+    sqfs_ls_at_symlink(u_boot_console)
+    sqfs_ls_at_non_existent_dir(u_boot_console)
+
 @pytest.mark.boardspec('sandbox')
 @pytest.mark.buildconfigspec('cmd_fs_generic')
 @pytest.mark.buildconfigspec('cmd_squashfs')
@@ -13,24 +59,22 @@ from sqfs_common import *
 @pytest.mark.requiredtool('mksquashfs')
 def test_sqfs_ls(u_boot_console):
     build_dir = u_boot_console.config.build_dir
-    for opt in comp_opts:
-        try:
-            opt.gen_image(build_dir)
-        except RuntimeError:
-            opt.clean_source(build_dir)
-            # skip unsupported compression types
-            continue
-        path = os.path.join(build_dir, "sqfs-" + opt.name)
-        output = u_boot_console.run_command("host bind 0 " + path)
 
+    # setup test environment
+    generate_sqfs_src_dir(build_dir)
+    make_all_images(build_dir)
+
+    # run all tests for each image
+    for image in standard_table.keys():
         try:
-            # list files in root directory
-            output = u_boot_console.run_command("sqfsls host 0")
-            assert str(len(opt.files) + 1) + " file(s), 0 dir(s)" in output
-            assert "<SYM>   sym" in output
-            output = u_boot_console.run_command("sqfsls host 0 xxx")
-            assert "** Cannot find directory. **" in output
+            image_path = os.path.join(build_dir, image)
+            u_boot_console.run_command('host bind 0 {}'.format(image_path))
+            sqfs_run_all_ls_tests(u_boot_console)
         except:
-            opt.cleanup(build_dir)
-            assert False
-        opt.cleanup(build_dir)
+            clean_all_images(build_dir)
+            clean_sqfs_src_dir(build_dir)
+            raise AssertionError
+
+    # clean test environment
+    clean_all_images(build_dir)
+    clean_sqfs_src_dir(build_dir)
-- 
2.25.1

Re: [PATCH 1/3] test/py: rewrite common tools for SquashFS tests

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

On Sun, May 23, 2021 at 11:31:31PM -0300, Joao Marcos Costa wrote:
> Remove the previous OOP approach, which was confusing and incomplete.
> Add more test cases by making SquashFS images with various options,
> concerning file fragmentation and its compression. Add comments to
> properly document the code.
> 
> Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
> ---
>  .../test_fs/test_squashfs/sqfs_common.py      | 198 ++++++++++++------
>  1 file changed, 133 insertions(+), 65 deletions(-)
> 
> diff --git a/test/py/tests/test_fs/test_squashfs/sqfs_common.py b/test/py/tests/test_fs/test_squashfs/sqfs_common.py
> index c96f92c1d8..81a378a9f9 100644
> --- a/test/py/tests/test_fs/test_squashfs/sqfs_common.py
> +++ b/test/py/tests/test_fs/test_squashfs/sqfs_common.py
> @@ -1,76 +1,144 @@
>  # SPDX-License-Identifier: GPL-2.0
> -# Copyright (C) 2020 Bootlin
> +# Copyright (C) 2021 Bootlin

Unless you have some different internal policy, it's typical to extend
copyright years, not replace them.  Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/3] test/py: Rewrite SquashFS commands test suite

[Richard Genoud]

Hi,
Le 24/05/2021 à 04:31, Joao Marcos Costa a écrit :
> Hello,
> 
> This patch series fixes the following issues:
> - poor strategy to check if files were properly loaded
> - wrong quoting style for strings
> - tests failing at the second run because of a wrong clean-up strategy
> 
> Finally, it improves:
> - code overall documentation level, with more comments and better
>    naming for functions and variables
> - code readability by adding more helper functions
> - completeness: more test cases were added for both sqfsls and sqfsload
>    commands
> 
> The sqfsload new test suite may fail when testing images with fragmented
> files if the patch I previously sent (fs/squashfs: fix reading of
> fragmented files) is not applied, so this patch series depends on it.
> 
> Best regards,
> Joao
> 
> Joao Marcos Costa (3):
>    test/py: rewrite common tools for SquashFS tests
>    test/py: rewrite sqfsload command test suite
>    test/py: rewrite sqfsls command test suite
> 
>   .../test_fs/test_squashfs/sqfs_common.py      | 198 ++++++++++++------
>   .../test_fs/test_squashfs/test_sqfs_load.py   |  99 ++++++---
>   .../test_fs/test_squashfs/test_sqfs_ls.py     |  80 +++++--
>   3 files changed, 264 insertions(+), 113 deletions(-)
> 

Tested-by: Richard Genoud <richard.genoud@posteo.net>


Thanks !

Re: [PATCH 1/3] test/py: rewrite common tools for SquashFS tests

[Simon Glass]

Hi Joao,

On Sun, 23 May 2021 at 20:31, Joao Marcos Costa <jmcosta944@gmail.com> wrote:
>
> Remove the previous OOP approach, which was confusing and incomplete.
> Add more test cases by making SquashFS images with various options,
> concerning file fragmentation and its compression. Add comments to
> properly document the code.
>
> Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
> ---
>  .../test_fs/test_squashfs/sqfs_common.py      | 198 ++++++++++++------
>  1 file changed, 133 insertions(+), 65 deletions(-)
>
> diff --git a/test/py/tests/test_fs/test_squashfs/sqfs_common.py b/test/py/tests/test_fs/test_squashfs/sqfs_common.py
> index c96f92c1d8..81a378a9f9 100644
> --- a/test/py/tests/test_fs/test_squashfs/sqfs_common.py
> +++ b/test/py/tests/test_fs/test_squashfs/sqfs_common.py
> @@ -1,76 +1,144 @@
>  # SPDX-License-Identifier: GPL-2.0
> -# Copyright (C) 2020 Bootlin
> +# Copyright (C) 2021 Bootlin
>  # Author: Joao Marcos Costa <joaomarcos.costa@bootlin.com>
>
>  import os
> -import random
> -import string
> +import shutil
>  import subprocess
>
> -def sqfs_get_random_letters(size):
> -    letters = []
> -    for i in range(0, size):
> -            letters.append(random.choice(string.ascii_letters))
> +"""
> +standard test images table: Each table item is a key:value pair representing the
> +output image name and its respective mksquashfs options. This table should be
> +modified only when adding support for new compression algorithms.
> +The 'default' case takes no options but the input and output names, so it must
> +be assigned with an empty string.
> +"""
> +standard_table = {
> +        'default' : '',
> +        'lzo_comp_frag' : '',
> +        'lzo_frag' : '',
> +        'lzo_no_frag' : '',
> +        'zstd_comp_frag' : '',
> +        'zstd_frag' : '',
> +        'zstd_no_frag' : '',
> +        'gzip_comp_frag' : '',
> +        'gzip_frag' : '',
> +        'gzip_no_frag' : ''
> +}
>
> -    return ''.join(letters)
> +"""
> +extra_table: Set this table's keys and values if you want to make squashfs images with
> +your own customized options.
> +"""
> +extra_table = {}
>
> -def sqfs_generate_file(path, size):
> -    content = sqfs_get_random_letters(size)
> -    file = open(path, "w")
> +# path to source directory used to make squashfs test images
> +sqfs_src_dir = 'sqfs_src_dir'
> +
> +"""
> +Combines fragmentation and compression options into a list of strings.
> +opts_list's firts item is an empty string as standard_table's first item is
> +the 'default' case.
> +"""
> +def get_opts_list():

While we are here, can you please be a little more formal with your
function comments? pylint3 will tell you about it if you run it on
this file.

Basically you do something like this (omitting Args or Returns if
there isn't anything):

def get_opts_list():
    """Combine fragmentation and compression options into a list of strings

   The first returned item is an empty string as standard_table's first item is
   the 'default' case.

   Args:
      fred: Description

   Returns:
      what it returns
   """

> +    # supported compression options only
> +    comp_opts = ['-comp lzo', '-comp zstd', '-comp gzip']
> +    # file fragmentation options
> +    frag_opts = ['-always-use-fragments', '-always-use-fragments -noF', '-no-fragments']
> +
> +    opts_list = [' ']
> +    for c in comp_opts:
> +        for f in frag_opts:
> +            opts_list.append(' '.join([c, f]))
> +
> +    return opts_list
> +
> +def init_standard_table():
> +    opts_list = get_opts_list()
> +
> +    for key, value in zip(standard_table.keys(), opts_list):
> +            standard_table[key] = value
> +
> +def generate_file(file_name, file_size):
> +    # file is filled with file_size * 'x'
> +    content = ''
> +    for i in range(file_size):
> +        content += 'x'

content = 'x' * file_size

> +
> +    file = open(file_name, 'w')
>      file.write(content)
>      file.close()
>
[..]

[..]

> +"""
> +sqfs_src_dir
> +├── empty-dir
> +├── f1000
> +├── f4096
> +├── f5096
> +├── subdir
> +│   └── subdir-file
> +└── sym -> subdir
> +
> +3 directories, 4 files
> +"""

That should be a comment for this function.

> +def generate_sqfs_src_dir(build_dir):

   """What this function does

   put above info here
   """

> +    path = os.path.join(build_dir, sqfs_src_dir)

Call it root ?

> +    print(path)
> +    #path = build_dir + '/' + sqfs_src_dir

Can we drop those two lines?

> +    # make root directory
> +    os.makedirs(path)
> +
> +    # 4096: minimum block size
> +    file_name = 'f4096'
> +    generate_file(os.path.join(path, file_name), 4096)
> +
> +    # 5096: minimum block size + 1000 chars (fragment)
> +    file_name = 'f5096'
> +    generate_file(os.path.join(path, file_name), 5096)
> +
> +    # 1000: less than minimum block size (fragment only)
> +    file_name = 'f1000'
> +    generate_file(os.path.join(path, file_name), 1000)
> +
> +    # sub-directory with a single file inside
> +    subdir_path = os.path.join(path, 'subdir')
> +    os.makedirs(subdir_path)
> +    generate_file(os.path.join(subdir_path, 'subdir-file'), 100)
> +
> +    # symlink (target: sub-directory)
> +    os.symlink('subdir', os.path.join(path, 'sym'))
> +
> +    # empty directory
> +    os.makedirs(os.path.join(path, 'empty-dir'))
> +
> +def mksquashfs(args):
> +    subprocess.run(['mksquashfs ' + args], shell = True, check = True,
> +            stdout = subprocess.DEVNULL)
> +
> +def make_all_images(build_dir):
> +
> +    init_standard_table()
> +    input_path = os.path.join(build_dir, sqfs_src_dir)
> +
> +    # make squashfs images according to standard_table
> +    for out, opts in zip(standard_table.keys(), standard_table.values()):
> +        output_path = os.path.join(build_dir, out)
> +        mksquashfs(' '.join([input_path, output_path, opts]))
> +
> +    # make squashfs images according to extra_table
> +    for out, opts in zip(extra_table.keys(), extra_table.values()):
> +        output_path = os.path.join(build_dir, out)
> +        mksquashfs(' '.join([input_path, output_path, opts]))
> +
> +def clean_all_images(build_dir):
> +    for image_name in standard_table.keys():
> +        image_path = os.path.join(build_dir, image_name)
> +        os.remove(image_path)
> +
> +    for image_name in extra_table.keys():
> +        image_path = os.path.join(build_dir, image_name)
> +        os.remove(image_path)
> +
> +def clean_sqfs_src_dir(build_dir):
> +    path = os.path.join(build_dir, sqfs_src_dir)
> +    shutil.rmtree(path)
> --
> 2.25.1
>

Regards,
Simon

Re: [PATCH 1/3] test/py: rewrite common tools for SquashFS tests

[João Marcos Costa]

Hello,

Em qui., 27 de mai. de 2021 às 10:44, Simon Glass <sjg@chromium.org>
escreveu:

> Hi Joao,
>
> On Sun, 23 May 2021 at 20:31, Joao Marcos Costa <jmcosta944@gmail.com>
> wrote:
> >
> > Remove the previous OOP approach, which was confusing and incomplete.
> > Add more test cases by making SquashFS images with various options,
> > concerning file fragmentation and its compression. Add comments to
> > properly document the code.
> >
> > Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
> > ---
> [...]
>
> While we are here, can you please be a little more formal with your
> function comments? pylint3 will tell you about it if you run it on
> this file.
>
> [...]
>
> Regards,
> Simon
>

Absolutely, Simon, and thanks for suggesting pylint3!

-- 
Atenciosamente,
João Marcos Costa

www.linkedin.com/in/jmarcoscosta/
https://github.com/jmarcoscosta