[RESEND PATCH 06/10] test: environment in ext4

[Stephen Warren]

On 2/12/20 11:44 AM, Patrick Delaunay wrote:
> Add basic test to persistent environment in ext4:
> save and load in host ext4 file 'uboot.env'.
> 
> On first execution a empty EXT4 file system is created in
> persistent data dir: env.ext4.img.

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

> +def mk_env_ext4(state_test_env):
> +    c = state_test_env.u_boot_console
> +
> +    """Create a empty ext4 file system volume.
> +    """
> +    filename = 'env.ext4.img'
> +    persistent = c.config.persistent_data_dir + '/' + filename

The test seems to want to delete this file at the end, and re-create it 
each time. Why store it in the persistent data directory in that case?

> +    if os.path.exists(persistent):
> +        c.log.action('Disk image file ' + persistent + ' already exists')

... and why just delete it and re-create it if a stale copy exists?

> +    else:
> +        try:
> +            check_call('rm -f %s' % persistent, shell=True)

why not:

     check_call('rm -f ' + persistent)

That has simpler string operations, and I don't think any of these 
commands need shell=True, since they don't contain any shell 
metacharacters? Same comment for most of the other commands.

> +def test_env_ext4(state_test_env):

> +    """ env_location: ENVL_EXT4 (2)
> +    """

I think that'd be better as a regular comment:

     # env_location: ENVL_EXT4 (2)

If there's a reason to make it a docstring, lets put the trailing """ on 
the same line since it's a short piece of text.

> +    response = c.run_command('env_loc 2')
> +    assert 'Saving Environment to EXT4' in response
> +
> +    response = c.run_command('env_loc 2')
> +    assert 'Loading Environment from EXT4... OK' in response

Can't both assert invocations run on the same response? Or, does the 
env_loc command do different things based on some other state; that 
would feel pretty nasty.