[U-Boot] [PATCH v1 1/3] test/py: gpt: update test_gpt

[Patrick DELAUNAY]

Hi Stephen,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> 
> On 10/09/2017 01:47 AM, Patrick Delaunay wrote:
> > - copy the reference gpt binary file as it is modified during the test
> >    to avoid issue if the test fail: the test always restart with clean
> > file
> > - update tests to highlight detected issues on gpt swap command
> >    (offset and size of partition are modified)
> > - add test for gpt verfiy, gpt read and gpt write
> 
> It might be nice to split this into separate patches for separate logical changes.

Ok I will split this patch in 3 parts in v2

> 
> > diff --git a/test/py/tests/test_gpt.py b/test/py/tests/test_gpt.py
> 
> >   These tests rely on a 4 MB disk image, which is automatically
> > created by -the test.
> > +the test, but as we expect specific content and it is modified by the
> > +gpt commands executed during the test, it is not safe be reused it
> > +directly
> 
> That's a bit of an internal detail. I'm not sure it's necessary to spell it out in the
> documentation?

Yes, I agree.
It is the first time I modify tets/py and I don't see the fact that these lines are not just comment.
I will remove it in v2

> 
> >   class GptTestDiskImage(object):
> > @@ -28,26 +29,31 @@ class GptTestDiskImage(object):
> >           """
> >
> >           filename = 'test_gpt_disk_image.bin'
> > +
> >           self.path = u_boot_console.config.persistent_data_dir + '/'
> > + filename
> > +        ref_path = u_boot_console.config.persistent_data_dir +
> > + '/ref_' + filename
> 
> Can we put self.path somewhere other than persistent_data_dir? Since it gets
> re-created every time, it's not persistent. Other tests use
> u_boot_console.config.result_dir for temporary files.

Agree, I search a location for temporary file, but I don't found a good solution...
So I will use u_boot_console.config.result_dir in v2

> 
> > @@ -64,6 +70,30 @@ def state_disk_image(u_boot_console):
> >   @pytest.mark.boardspec('sandbox')
> >   @pytest.mark.buildconfigspec('cmd_gpt')
> >   @pytest.mark.requiredtool('sgdisk')
> > +def test_gpt_read(state_disk_image, u_boot_console):
> > +    """Test the gpt read command."""
> > +
> > +    u_boot_console.run_command('host bind 0 ' + state_disk_image.path)
> > +    output = u_boot_console.run_command('gpt read host 0')
> > +    assert 'Start 1MiB, size 0MiB' in output
> > +    assert 'Start 2MiB, size 0MiB' in output
> > +    output = u_boot_console.run_command('part list host 0')
> 
> I think this test should also be:
> @pytest.mark.buildconfigspec('cmd_part')
> 
> Some of the other diffs in this path add use of the part command for the first
> time, so I think you need to add that decorator to a few other functions too.

Ok

> 
> > @@ -109,9 +142,29 @@ def test_gpt_swap_partitions(state_disk_image,
> u_boot_console):
> >
> >       u_boot_console.run_command('host bind 0 ' + state_disk_image.path)
> >       output = u_boot_console.run_command('part list host 0')
> > -    assert '0x000007ff	"first"' in output
> > -    assert '0x000017ff	"second"' in output
> > +    assert '0x00000800	0x00000a00	"first"' in output
> > +    assert '0x00001000	0x00001200	"second"' in output
> 
> I'm not sure why this change is required. I can see two separate changes
> here:
> 
> 1) Verifies the value of 3 columns of data not just 2. That seems fine.
> 
> 2) Changes the expected value in the column before the partition name.
> I'm not sure about this; shouldn't the value stay the same, or is the test failing
> right now?

in fact it the purpose of my modification in patch 3/3 : cmd: gpt: solve issue for swap

The command gpt swap (or gpt remane) don't expect change the offset or the size of the partition
but it is the case today , it is an error in code and it is not detected with the current test

In the modified test, I test the expected correct values (3 columns): without modification of the offset and size.

=> the test if failing for master branch
=> the test is OK only after " patch 3/3 : cmd: gpt: solve issue for swap"

But I don't sure of the correct patchset order to avoid issue during merge in master branch

First Test update  => test Failed
Then patch the code => test OK

Or 

First patch the code => test Failed
Then patch the test => test OK

Patrick