[U-Boot] [PATCH v2 2/2] test/py: Create tests for ext4 and fat testing on sandbox

[Stephen Warren]

On 12/04/2016 05:52 PM, Stefan Brüns wrote:
> From: Stefan Brüns <stefan.bruens at rwth-aachen.de>
>
> The following checks are currently implemented:
> 1. listing a directory
> 2. verifying size of a file
> 3. veryfying md5sum for a file region
> 4. reading the beginning of a file

General comments:

1) I don't see anywhere that limits this test to sandbox. I assume it 
can't run on non-sandbox targets, even when using the U-Boot native 
filesystem commands, since there's no logic here to transfer the 
filesystem images to the target device.

I think this issue blocks checking in this change as-is, although all 
the other comments below can likely be deferred until later if you want, 
perhaps with the exception of (2) immediately below?

2) Doesn't mounting/unmounting require root? Nothing else in test/py 
does. It'd be useful to put a comment at the top of the file detailing 
which command one might want to add to /etc/sudoers to avoid having to 
run the whole thing as root, and use sudo within the test where necessary.

> diff --git a/test/py/tests/test_fs.py b/test/py/tests/test_fs.py

> +from distutils.spawn import find_executable

Is distutils part of the standard library, i.e. in a default install? If 
not, please add it to the requirements section of test/py/README.md.

> +"""
> +Scenarios:
> +    hostfs: access image contents through the sandbox hostfs
> +        facility, using the filesytem implementation of
> +        the sandbox host, e.g. Linux kernel
> +    generic: test u-boots native filesystem implementations,
> +        using the 'generic' command names, e.g. 'load'
> +    TODO -
> +    fscommands: test u-boots native filesystem implementations,
> +        using the fs specific commands, e.g. 'ext4load'
> +"""
> + at pytest.fixture(scope='class', params=['generic', 'hostfs'])
> +def scenario(request):

Shouldn't that docstring be the first "statement" of the function, not 
right before it? I can't remember if Python picks up docstrings from 
right before objects too. If not, perhaps a comment would be better 
rather than a string.

> +class FsImage:
...
> +    def mount(self, log):
> +        if not os.path.exists(self.mountpath):
> +            os.mkdir(self.mountpath)

Use os.makedirs(path) instead; you can avoid the if statement, and it'll 
handle parent directories too.

> + at pytest.fixture(scope='module', params=['fat', 'ext4'])
> +def fsimage(prereq_commands, u_boot_config, u_boot_log, request):
> +    """Filesystem image instance."""
> +    datadir = u_boot_config.result_dir + '/'

Wouldn't it be better to put this into 
u_boot_config.persistent_data_dir, plus avoid creating the image file if 
it already exists? See u_boot_utils.py's PersistentRandomFile() as an 
example. I wonder if that could be expanded to create files not just of 
size n, but with sparse layout specs like this test uses?

> +    fstype = request.param
> +    imagepath = datadir + '3GB.' + fstype + '.img'
> +    mountpath = datadir + 'mnt_' + fstype
> +
> +    with u_boot_log.section('Create image "{0}"'.format(imagepath)):
> +        fsimage = FsImage(fstype, imagepath, mountpath)
> +        fsimage.mkfs(u_boot_log)
> +
> +    yield fsimage
> +    fsimage.unmount(u_boot_log)

Unmounting seems to happen in a lot of different places. Can we isolate 
it to just one place?

Also, what happens if the code throws an exception after obtaining an 
fsimage from this generator; I'm not sure that any cleanup happens in 
that case. Should there be "try: ... finally: unmount()" somewhere to 
clean up even in the case of an error? Alternatively, perhaps class 
FsImage should have a destructor that does the unmount (at least if it 
hasn't happened already)?

> +
> +
> +def test_fs_prepare_image(u_boot_config, fsimage, request):

Nit: Two blank lines there.

> +    """Dummy test to create an image file with filesystem.
> +    Useful to isolate fixture setup from actual tests."""
> +    if not fsimage:
> +        pytest.fail('Failed to create image')
> +
> +def test_fs_populate_image(populated_image, request):
> +    """Dummy test to create initial filesystem contents."""
> +    if not populated_image:
> +        pytest.fail('Failed create initial image content')

Why not just fail (or raise an exception) right where the error occurs?

> +    def run_readcmd(self, filename, offset, length):
> +        """Run the scenario and filesystem specific read command
> +        for the given file."""
> +        cmd = '{0}{1} {2} {3} {4} 0x{5:x} 0x{6:x}'.format(
> +            self.fs_params.get('prefix'),
> +            self.fs_commands.get('readcmd'),
> +            self.fs_params.get('interface'),
> +            '0', # address

It might be a good idea to invoke 
u_boot_utils.find_ram_base(u_boot_console) rather than hard-coding an 
address of 0. I don't know if sandbox will ever change its memory 
layout, but if there's any chance any of this will be ported to real HW, 
that function will be required.

> +            self.image.rootpath + filename,
> +            length, offset)
> +        with self.console.log.section('Read file "{0}"'.format(filename)):
> +            output = self.console.run_command_list(
> +                [cmd, 'env print filesize',
> +                 'md5sum 0 $filesize', 'env set filesize'])

Out of curiosity, why not "echo $filesize"?

> +            return output[1:3]

No error checking for output[0]? I suppose if u_boot_console_base.py's 
bad_pattern_defs[] included error patterns that "readcmd" was expected 
to emit, that'd be fine, but it doesn't currently. Maybe we expect that 
the other command can't possibly succeed if the read doesn't. Similar 
comment for run_sizecmd() below, and perhaps elsewhere.

> +    @pytest.mark.parametrize('dirname', ['', './'])
> +    def test_fs_ls(self, u_boot_console, dirname):
> +        """Check the contents of the given directory."""
> +        if self.image.fstype == 'fat' and dirname == './':
> +            pytest.skip("FAT has no '.' entry in the root directory")

Is there a "/" or "" prefix tested too? Hopefully the root dir gets 
tested on FAT.