Hi,
> From: Stephen Warren
> Sent: lundi 22 juin 2020 20:57
>
> On 6/16/20 1:40 AM, Patrick Delaunay wrote:
> > Add basic test to persistent environment in ext4:
> > save and load in host ext4 file 'uboot.env'.
> >
> > On first execution an 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):
>
> > +if os.path.exists(persistent):
> > +c.log.action('Disk image file ' + persistent + ' already exists')
> > +else:
> > +try:
> > +check_call('rm -f %s' % persistent, shell=True)
>
> This should be run with the results logged to the overall test log file so
> that if there
> are failures, it's possible to see what they were. Use
> util.run_and_log() for this.
Ok, I will modifiy this part (I copy ext4 file create is copied form
py/tests/test_fs/conftest.py:166 mk_fs() )
> Also, this particular command doesn't seem useful, since 4 lines above the
> code
> verified that the file doesn't exist.
Yes not needed, I remove this line.
> > +@pytest.mark.boardspec('sandbox')
> > +@pytest.mark.buildconfigspec('cmd_nvedit_info')
> > +@pytest.mark.buildconfigspec('cmd_echo')
> > +@pytest.mark.buildconfigspec('env_is_in_ext4')
> > +def test_env_ext4(state_test_env):
> > +
> > +c = state_test_env.u_boot_console
>
> Nit: That blank line is a bit odd.
OK
> > +fs_img = mk_env_ext4(state_test_env)
> > +c.run_command('host bind 0 %s' % fs_img)
> > +
> > +response = c.run_command('ext4ls host 0:0')
> > +assert 'uboot.env' not in response
> > +
> > +""" env location: ENVL_EXT4 (2)
> > +"""
>
> Nit: Wrap the trailing """ onto the same line; no need to force it to be a
> multi-line
> string. Also a comman may be better rather than a docstring. Same for the
> other
> docstring later.
OK, I don't realized that it was docstring.
> > +call('rm -f %s' % fs_img, shell=True)
>
> This won't happen if the test fails earlier. Should there be a try/except
> block or
> wrapper function with exception handling to resolve this?
Yes, I add it for V3
Patrick
