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

[Stefan Bruens]

On Montag, 12. Dezember 2016 11:04:34 CET you wrote:
> 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 have added Sandbox only for next patch version.

Running it from qemu should not be to difficult, "transferring" should be 
possible by adding the image to the qemu command.

Ideally, the FS image would be created once and then be reused. This had 
several benefits:
1. Reducing time for creation of the FAT image(s). Creation of the 2.5GB test 
file is time consuming, as FAT has no sparse files. Storage requirement is 
quite low if the FAT image is "resparsed" e.g. using "copy --sparse=always".
2. Sudo would be only (mostly) needed for the initial image creation.
3. If the image were created from a template, write tests (to be added later) 
could always start with a pristine image.

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

It already uses sudo, the run command is visible from the log. Currently, I 
just run "sudo true" prior to executing the test, the password is cached for 5 
minutes.

If the image were created once, there would be no need for sudo for later runs 
(save hostfs).
 
> > 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.

Don't know for other distros, for openSUSE it is.
 
> > +"""
> > +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.

For attributes, the docstring should be in front of the initial assignment, 
and may be picked up by the doc generator. For functions, it should be the 
first line(s) of the function.
 
> > +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.

exist_ok only exists in python >= 3.2.
 
> > + 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?

See above. I don't think PersistentRandomFile() does fit here, as the files 
are created *inside* an image. Maybe it could be used after the image and 
filesystem  is created and mounted ...
 
> > +    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?

The image is mounted/unmounted for two different reasons - creating/populating 
the image, and when accessing it using the hostfs commands.
 
> 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)?

Its a fixture and will be torn down by pytest.
 
> > +
> > +
> 
> > +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"?

Because "filesize=1234" is nicer to match
 
> > +            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.

filesize is only set if the read succeeds, and "env print filesize" matches 
bad_pattern_defs in case of an error.
 
> > +    @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.

See first parametrize entry.

Kind regards,

Stefan


-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424