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

[Stefan Bruens]

On Donnerstag, 5. Januar 2017 17:15:02 CET you wrote:
> On 01/01/2017 02:48 PM, Stefan Bruens wrote:
> > 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
> >> 
> >> 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.
> 
> I don't see sudo used anywhere in test/py; can you point out where you
> see it using sudo at present?

it == the fs test
 
> >>> +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.
> 
> There's no need to use exist_ok; do this (example from conftest.py):
> 
>      try:
>          os.makedirs(path)
>      except OSError as exc:
>          if exc.errno == errno.EEXIST and os.path.isdir(path):
>              pass
>          else:
>              raise

Thats not what you wrote. You said "you can avoid the if statement". What you 
meant is "you can replace the single line if statement with a 6 line exception 
handling statement". The latter is *only* better iff you have to handle 
concurrent creation of the wanted directory.
 
> >>> + 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 ...
> 
> You missed my point. PersistentRandomFile() is existing code that
> creates a persistent data file. You can create a new function/... that
> uses the same techniques to create the data file, but make it create a
> filesystem image rather than random data.

The disk image is a fixture, so it should be created as a fixture. The usage 
of PersistentRandomFile as currently done is wrong, it is a prerequisite of 
the test, not part of the test itself.

The correct use of PersistentRandomFile might be as a helper function used by 
different fixtures.

1. persistent_data_dir may be the better choice, but only if the image is 
created as a template there and then copied to a different location for each 
test run - the idea is to modify the image by write tests, and a clean image 
is needed here. Feel free to implement this yourself.

2. PersistentRandomFile creates single files, the only parameter is the size 
(although extending it to sparse files is quite possible). Disk images with 
filesystems have a lot more parameters - filesystem itself, filesystem 
options, filesystem contents.

Actually you missed my point here - PersistentRandomFile() is existing code, 
but it is useless here. Shoehorning it in here or extending it until it suits 
is a bad idea. It does one thing, and it does it fine. Creation of disk images 
is a different thing it does and should not do.

> >>> +    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.
> 
> IIRC, there's more duplication than that, but I'll look again when this
> is reposted.
> 
> >> 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.
> 
> How does that work? Once the fixture function has yielded the image,
> surely if an exception is thrown, it'll be "thrown through" the
> generator function and hence prevent the rest of the function body from
> running? Or is there some special magic that lets the generator
> complete, even if the yield effectively threw an exception?

Please read up on pytest:
http://pytest.org/dev/yieldfixture.html

> >>> +            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.
> 
> What pattern is printed that matches bad_pattern_defs? Nothing in
> bad_pattern_defs obviously would get printed if $filesize wasn't set.

=> env print foo              
## Error: "foo" not defined

> Also, what if $filesize is still set from an earlier test (e.g. in a
> different Python file), and a later test fails, yet this isn't detected
> since that relies on $filesize not being set? I see this script doing
> "env set filesize" /after/ its own tests which should prevent this, but
> not /before/ the first test.

There you have a point. *If* filesize where set, *and* the previously set 
value matches the expected value *and* this is the very first test to check 
$filesize, an error would have gone unnoticed ...

Regards,

Stefan


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