[Buildroot] [PATCH v3 3/5] support/testing: add fs tests
Luca Ceresoli
luca at lucaceresoli.net
Sun May 7 20:55:48 UTC 2017
Hi,
this the review I had written a few days ago. Since the patch has just
been applied, I've reworded it as a list of proposed improvements and
request for clarifications.
On 20/03/2017 21:36, Thomas Petazzoni wrote:
> This commit adds a number of test cases for various filesystem formats:
> ext2/3/4, iso9660, jffs2, squashfs, ubi/ubifs and yaffs2. All of them
> except yaffs2 are runtime tested. The iso9660 set of test cases is
> particularly rich, testing the proper operation of the iso9660 support
> with all of grub, grub2 and isolinux.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni at free-electrons.com>
> ---
> support/testing/conf/grub-menu.lst | 20 +++
> support/testing/conf/grub2.cfg | 7 +
> support/testing/conf/isolinux.cfg | 5 +
> .../testing/conf/minimal-x86-qemu-kernel.config | 23 +++
> support/testing/tests/fs/__init__.py | 0
> support/testing/tests/fs/test_ext.py | 119 +++++++++++++++
> support/testing/tests/fs/test_iso9660.py | 162 +++++++++++++++++++++
> support/testing/tests/fs/test_jffs2.py | 45 ++++++
> support/testing/tests/fs/test_squashfs.py | 37 +++++
> support/testing/tests/fs/test_ubi.py | 39 +++++
> support/testing/tests/fs/test_yaffs2.py | 12 ++
> 11 files changed, 469 insertions(+)
> create mode 100644 support/testing/conf/grub-menu.lst
> create mode 100644 support/testing/conf/grub2.cfg
> create mode 100644 support/testing/conf/isolinux.cfg
I suspect it's already been suggested, but I would rather put these
files in the 'fs' subdirectory, unless we expect to use them for non-fs
tests.
Do other people agree? Should I send a patch?
> diff --git a/support/testing/tests/fs/test_ext.py b/support/testing/tests/fs/test_ext.py
> new file mode 100644
> index 0000000..f7e2e85
> --- /dev/null
> +++ b/support/testing/tests/fs/test_ext.py
[...]
> +def dumpe2fs_getprop(out, prop):
> + for lines in out:
Here 'lines' is set to a string (one line)...
> + lines = lines.split(": ")
...and here it is reassigned to a list (the line fields, as they are
called in awk. Or is the second assignment of 'lines' creating a new
variable with the same name? Whether the reply, I don't like this.
> + if lines[0] == prop:
> + return lines[1].strip()
My proposal (tested):
def dumpe2fs_getprop(out, prop):
for line in out:
fields = line.split(": ")
if fields[0] == prop:
return fields[1].strip()
Unless there's a good reason to keep the code as is, I'll send a patch
anyway, to improve readability at least.
> +def boot_img_and_check_fs_type(emulator, builddir, fs_type):
> + img = os.path.join(builddir, "images", "rootfs.{}".format(fs_type))
Here (and similarly in several other places) this looks a lot more
intuitive and concise to me:
"rootfs." + fs_type
Any good reason the initial form is preferred? Pythonists out there?
[...]
> +class TestExt3(infra.basetest.BRTest):
> + config = infra.basetest.BASIC_TOOLCHAIN_CONFIG + \
> +"""
> +BR2_TARGET_ROOTFS_EXT2=y
> +BR2_TARGET_ROOTFS_EXT2_3=y
> +# BR2_TARGET_ROOTFS_TAR is not set
> +"""
> +
> + def test_run(self):
> + out = dumpe2fs_run(self.builddir, "rootfs.ext3")
> + self.assertEqual(dumpe2fs_getprop(out, REVISION_PROP), "1 (dynamic)")
> + self.assertIn("has_journal", dumpe2fs_getprop(out, FEATURES_PROP))
> +
> + exit_code = boot_img_and_check_fs_type(self.emulator,
> + self.builddir, "ext3")
> + self.assertEqual(exit_code, 0)
> +
> +class TestExt4(infra.basetest.BRTest):
> + config = infra.basetest.BASIC_TOOLCHAIN_CONFIG + \
> +"""
> +BR2_TARGET_ROOTFS_EXT2=y
> +BR2_TARGET_ROOTFS_EXT2_4=y
> +BR2_TARGET_ROOTFS_EXT2_BLOCKS=16384
> +BR2_TARGET_ROOTFS_EXT2_INODES=3000
> +BR2_TARGET_ROOTFS_EXT2_RESBLKS=10
> +# BR2_TARGET_ROOTFS_TAR is not set
> +"""
> +
> + def test_run(self):
> + out = dumpe2fs_run(self.builddir, "rootfs.ext4")
> + self.assertEqual(dumpe2fs_getprop(out, REVISION_PROP), "1 (dynamic)")
> + self.assertEqual(dumpe2fs_getprop(out, BLOCKCNT_PROP), "16384")
> + # Yes there are 8 more inodes than requested
> + self.assertEqual(dumpe2fs_getprop(out, INODECNT_PROP), "3008")
> + self.assertEqual(dumpe2fs_getprop(out, RESBLKCNT_PROP), "1638")
> + self.assertIn("has_journal", dumpe2fs_getprop(out, FEATURES_PROP))
> + self.assertIn("extent", dumpe2fs_getprop(out, FEATURES_PROP))
It would be nice to have both positive and a negative test for each
tested item. Specifically, I'd add self.assertIn("extent", ...) for ext3.
I'll send a patch for this.
> diff --git a/support/testing/tests/fs/test_iso9660.py b/support/testing/tests/fs/test_iso9660.py
> new file mode 100644
> index 0000000..eec6e89
> --- /dev/null
> +++ b/support/testing/tests/fs/test_iso9660.py
[...]
> +#
> +# Syslinux
> +#
> +
> +class TestIso9660SyslinuxExternal(infra.basetest.BRTest):
The two syslinux tests are failing on top of current master. This is
since commit 6e432d5ecb46 ("syslinux: build with the target toolchain").
The error message is:
.../i686-linux-ld: unrecognized option '--no-dynamic-linker'
> diff --git a/support/testing/tests/fs/test_squashfs.py b/support/testing/tests/fs/test_squashfs.py
> new file mode 100644
> index 0000000..edaa087
> --- /dev/null
> +++ b/support/testing/tests/fs/test_squashfs.py
[...]
> + def test_run(self):
> + unsquashfs_cmd = ["host/usr/bin/unsquashfs", "-s", "images/rootfs.squashfs"]
> + out = subprocess.check_output(unsquashfs_cmd,
> + cwd=self.builddir,
> + env={"LANG": "C"})
> + out = out.splitlines()
> + self.assertEqual(out[0],
> + "Found a valid SQUASHFS 4:0 superblock on images/rootfs.squashfs.")
> + self.assertEqual(out[3], "Compression lz4")
> +
> + img = os.path.join(self.builddir, "images", "rootfs.squashfs")
> + subprocess.call(["truncate", "-s", "%1M", img])
Why do we need this call to truncate?
--
Luca
More information about the buildroot
mailing list