[U-Boot] mmc tests incorrectly implemented

[U-Boot] mmc tests incorrectly implemented

[Stephen Warren]

I see that some mmc tests have been added to test/py, but I see problems
with them:

1) test_mmc_rescan assumes that each entry in env__mmc_rd_configs is a
separate device that can be rescanned. This isn't actually true; entries
in that array are intended to drive the mmc read test, and so can point
at partitions or specific sector numbers. Running an mmc rescan test on
the entire array results in duplicated tests. A new data array should be
created for different tests.

2) It relies on test data that was not previously required. In
particular, I see new test failures because the "info_device" key is now
required. New tests should either (a) use new optional data arrays to
configure their operation, or (b) access any new data in an optional
way, skipping the test if it's not present, so as not to cause test
failures.

Also, shouldn't things like new tests be announced to the board
maintainers list, so that people can update their test/py configurations
to enable new tests if they're appropriate for their platform?

[-] Section: test_mmc_info[emmc-boot0]
FAILED:
u_boot_console = <u_boot_console_exec_attach.ConsoleExecAttach object at
0x7fafd88f2d10>
env__mmc_rd_config = {'count': 1, 'devid': 0, 'fixture_id':
'emmc-boot0', 'is_emmc': True, ...}

    @pytest.mark.buildconfigspec('cmd_mmc')
    def test_mmc_info(u_boot_console, env__mmc_rd_config):
        """Test the "mmc info" command.

        Args:
            u_boot_console: A U-Boot console connection.
            env__mmc_rd_config: The single MMC configuration on which
                to run the test. See the file-level comment above for
details
                of the format.

        Returns:
            Nothing.
        """

        is_emmc = env__mmc_rd_config['is_emmc']
        devid = env__mmc_rd_config['devid']
        partid = env__mmc_rd_config.get('partid', 0)
>       info_device = env__mmc_rd_config['info_device']
E       KeyError: 'info_device'

src/u-boot/test/py/tests/test_mmc_rd.py:151: KeyError

[U-Boot] mmc tests incorrectly implemented

[Marek Vasut]

On 4/10/19 5:12 PM, Stephen Warren wrote:

Hi,

it would be nice if I was CCed on this.

> I see that some mmc tests have been added to test/py, but I see problems
> with them:
> 
> 1) test_mmc_rescan assumes that each entry in env__mmc_rd_configs is a
> separate device that can be rescanned. This isn't actually true; entries
> in that array are intended to drive the mmc read test, and so can point
> at partitions or specific sector numbers.

Is that documented somewhere ? I assumed they are separate devices and
if you need to test multiple partitions, you will have multiple entries
in this array, one for each device:partition pair.

> Running an mmc rescan test on
> the entire array results in duplicated tests. A new data array should be
> created for different tests.

I don't have such a usecase, but the fix should be easy to implement.
Can you do that ?

> 2) It relies on test data that was not previously required. In
> particular, I see new test failures because the "info_device" key is now
> required. New tests should either (a) use new optional data arrays to
> configure their operation, or (b) access any new data in an optional
> way, skipping the test if it's not present, so as not to cause test
> failures.

Either that, or update your tests. Can you submit a patch for this ?

> Also, shouldn't things like new tests be announced to the board
> maintainers list, so that people can update their test/py configurations
> to enable new tests if they're appropriate for their platform?

I don't know , should it ?

> [-] Section: test_mmc_info[emmc-boot0]
> FAILED:
> u_boot_console = <u_boot_console_exec_attach.ConsoleExecAttach object at
> 0x7fafd88f2d10>
> env__mmc_rd_config = {'count': 1, 'devid': 0, 'fixture_id':
> 'emmc-boot0', 'is_emmc': True, ...}
> 
>     @pytest.mark.buildconfigspec('cmd_mmc')
>     def test_mmc_info(u_boot_console, env__mmc_rd_config):
>         """Test the "mmc info" command.
> 
>         Args:
>             u_boot_console: A U-Boot console connection.
>             env__mmc_rd_config: The single MMC configuration on which
>                 to run the test. See the file-level comment above for
> details
>                 of the format.
> 
>         Returns:
>             Nothing.
>         """
> 
>         is_emmc = env__mmc_rd_config['is_emmc']
>         devid = env__mmc_rd_config['devid']
>         partid = env__mmc_rd_config.get('partid', 0)
>>       info_device = env__mmc_rd_config['info_device']
> E       KeyError: 'info_device'
> 
> src/u-boot/test/py/tests/test_mmc_rd.py:151: KeyError
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
> 


-- 
Best regards,
Marek Vasut

[U-Boot] mmc tests incorrectly implemented

[Tom Rini]

On Wed, Apr 10, 2019 at 06:23:46PM +0200, Marek Vasut wrote:
> On 4/10/19 5:12 PM, Stephen Warren wrote:
> 
> Hi,
> 
> it would be nice if I was CCed on this.
> 
> > I see that some mmc tests have been added to test/py, but I see problems
> > with them:
> > 
> > 1) test_mmc_rescan assumes that each entry in env__mmc_rd_configs is a
> > separate device that can be rescanned. This isn't actually true; entries
> > in that array are intended to drive the mmc read test, and so can point
> > at partitions or specific sector numbers.
> 
> Is that documented somewhere ? I assumed they are separate devices and
> if you need to test multiple partitions, you will have multiple entries
> in this array, one for each device:partition pair.
> 
> > Running an mmc rescan test on
> > the entire array results in duplicated tests. A new data array should be
> > created for different tests.
> 
> I don't have such a usecase, but the fix should be easy to implement.
> Can you do that ?
> 
> > 2) It relies on test data that was not previously required. In
> > particular, I see new test failures because the "info_device" key is now
> > required. New tests should either (a) use new optional data arrays to
> > configure their operation, or (b) access any new data in an optional
> > way, skipping the test if it's not present, so as not to cause test
> > failures.
> 
> Either that, or update your tests. Can you submit a patch for this ?
> 
> > Also, shouldn't things like new tests be announced to the board
> > maintainers list, so that people can update their test/py configurations
> > to enable new tests if they're appropriate for their platform?
> 
> I don't know , should it ?

On me, yes, this is probably something worth poking folks on the board
maintainers ML about, especially once it's shown useful on two SoCs :)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190410/57bbf049/attachment.sig>

[U-Boot] mmc tests incorrectly implemented

[Stephen Warren]

On 4/10/19 10:23 AM, Marek Vasut wrote:
> On 4/10/19 5:12 PM, Stephen Warren wrote:
> 
> Hi,
> 
> it would be nice if I was CCed on this.

Sorry, I didn't drill down in Jenkins/git data to find out where the
commits came from; I just had a list of commit descriptions and knew
which branch they showed up in,

>> I see that some mmc tests have been added to test/py, but I see problems
>> with them:
>>
>> 1) test_mmc_rescan assumes that each entry in env__mmc_rd_configs is a
>> separate device that can be rescanned. This isn't actually true; entries
>> in that array are intended to drive the mmc read test, and so can point
>> at partitions or specific sector numbers.
> 
> Is that documented somewhere ? I assumed they are separate devices and
> if you need to test multiple partitions, you will have multiple entries
> in this array, one for each device:partition pair.

There is an example in a comment at the top of test_mmc_rd.py, since
that's what is intended to use this data. Admittedly, it doesn't
explicitly spell out that the data array is intended for sole use by
that test, but I assumed it was obvious enough since the array was named
after the test and only used/mentioned in that one tests.

>> Running an mmc rescan test on
>> the entire array results in duplicated tests. A new data array should be
>> created for different tests.
> 
> I don't have such a usecase, but the fix should be easy to implement.
> Can you do that ?

Surely you have a use-case for the new test, or you wouldn't have
implemented it?

I'm afraid I barely have time to keep the test system running. I don't
have time to patch up test failures in most cases. The only option I
have available is to disable all MMC testing on my boards if that's
what's needed to keep the test system going to other tests. Sorry.

>> 2) It relies on test data that was not previously required. In
>> particular, I see new test failures because the "info_device" key is now
>> required. New tests should either (a) use new optional data arrays to
>> configure their operation, or (b) access any new data in an optional
>> way, skipping the test if it's not present, so as not to cause test
>> failures.
> 
> Either that, or update your tests. Can you submit a patch for this ?
> 
>> Also, shouldn't things like new tests be announced to the board
>> maintainers list, so that people can update their test/py configurations
>> to enable new tests if they're appropriate for their platform?
> 
> I don't know , should it ?

Well, why not? How else are people expected to find out about new tests
and know to enable them on their HW if appropriate? I certainly don't
think that expecting people running test systems to read every single
email or commit message to poll for changes to the tests is reasonable.

>> [-] Section: test_mmc_info[emmc-boot0]
>> FAILED:
>> u_boot_console = <u_boot_console_exec_attach.ConsoleExecAttach object at
>> 0x7fafd88f2d10>
>> env__mmc_rd_config = {'count': 1, 'devid': 0, 'fixture_id':
>> 'emmc-boot0', 'is_emmc': True, ...}
>>
>>     @pytest.mark.buildconfigspec('cmd_mmc')
>>     def test_mmc_info(u_boot_console, env__mmc_rd_config):
>>         """Test the "mmc info" command.
>>
>>         Args:
>>             u_boot_console: A U-Boot console connection.
>>             env__mmc_rd_config: The single MMC configuration on which
>>                 to run the test. See the file-level comment above for
>> details
>>                 of the format.
>>
>>         Returns:
>>             Nothing.
>>         """
>>
>>         is_emmc = env__mmc_rd_config['is_emmc']
>>         devid = env__mmc_rd_config['devid']
>>         partid = env__mmc_rd_config.get('partid', 0)
>>>       info_device = env__mmc_rd_config['info_device']
>> E       KeyError: 'info_device'
>>
>> src/u-boot/test/py/tests/test_mmc_rd.py:151: KeyError

[U-Boot] mmc tests incorrectly implemented

[Tom Rini]

On Wed, Apr 10, 2019 at 11:13:32AM -0600, Stephen Warren wrote:
> On 4/10/19 10:23 AM, Marek Vasut wrote:
> > On 4/10/19 5:12 PM, Stephen Warren wrote:
> > 
> > Hi,
> > 
> > it would be nice if I was CCed on this.
> 
> Sorry, I didn't drill down in Jenkins/git data to find out where the
> commits came from; I just had a list of commit descriptions and knew
> which branch they showed up in,
> 
> >> I see that some mmc tests have been added to test/py, but I see problems
> >> with them:
> >>
> >> 1) test_mmc_rescan assumes that each entry in env__mmc_rd_configs is a
> >> separate device that can be rescanned. This isn't actually true; entries
> >> in that array are intended to drive the mmc read test, and so can point
> >> at partitions or specific sector numbers.
> > 
> > Is that documented somewhere ? I assumed they are separate devices and
> > if you need to test multiple partitions, you will have multiple entries
> > in this array, one for each device:partition pair.
> 
> There is an example in a comment at the top of test_mmc_rd.py, since
> that's what is intended to use this data. Admittedly, it doesn't
> explicitly spell out that the data array is intended for sole use by
> that test, but I assumed it was obvious enough since the array was named
> after the test and only used/mentioned in that one tests.
> 
> >> Running an mmc rescan test on
> >> the entire array results in duplicated tests. A new data array should be
> >> created for different tests.
> > 
> > I don't have such a usecase, but the fix should be easy to implement.
> > Can you do that ?
> 
> Surely you have a use-case for the new test, or you wouldn't have
> implemented it?
> 
> I'm afraid I barely have time to keep the test system running. I don't
> have time to patch up test failures in most cases. The only option I
> have available is to disable all MMC testing on my boards if that's
> what's needed to keep the test system going to other tests. Sorry.

Wait, these are running and failing on your setup?  That wasn't clear,
sorry.  These showed up as skips on my setup as I haven't tried to
enable them here yet.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190410/febc285b/attachment.sig>

[U-Boot] mmc tests incorrectly implemented

[Stephen Warren]

On 4/10/19 11:22 AM, Tom Rini wrote:
> On Wed, Apr 10, 2019 at 11:13:32AM -0600, Stephen Warren wrote:
>> On 4/10/19 10:23 AM, Marek Vasut wrote:
>>> On 4/10/19 5:12 PM, Stephen Warren wrote:
>>>
>>> Hi,
>>>
>>> it would be nice if I was CCed on this.
>>
>> Sorry, I didn't drill down in Jenkins/git data to find out where the
>> commits came from; I just had a list of commit descriptions and knew
>> which branch they showed up in,
>>
>>>> I see that some mmc tests have been added to test/py, but I see problems
>>>> with them:
>>>>
>>>> 1) test_mmc_rescan assumes that each entry in env__mmc_rd_configs is a
>>>> separate device that can be rescanned. This isn't actually true; entries
>>>> in that array are intended to drive the mmc read test, and so can point
>>>> at partitions or specific sector numbers.
>>>
>>> Is that documented somewhere ? I assumed they are separate devices and
>>> if you need to test multiple partitions, you will have multiple entries
>>> in this array, one for each device:partition pair.
>>
>> There is an example in a comment at the top of test_mmc_rd.py, since
>> that's what is intended to use this data. Admittedly, it doesn't
>> explicitly spell out that the data array is intended for sole use by
>> that test, but I assumed it was obvious enough since the array was named
>> after the test and only used/mentioned in that one tests.
>>
>>>> Running an mmc rescan test on
>>>> the entire array results in duplicated tests. A new data array should be
>>>> created for different tests.
>>>
>>> I don't have such a usecase, but the fix should be easy to implement.
>>> Can you do that ?
>>
>> Surely you have a use-case for the new test, or you wouldn't have
>> implemented it?
>>
>> I'm afraid I barely have time to keep the test system running. I don't
>> have time to patch up test failures in most cases. The only option I
>> have available is to disable all MMC testing on my boards if that's
>> what's needed to keep the test system going to other tests. Sorry.
> 
> Wait, these are running and failing on your setup?  That wasn't clear,
> sorry.  These showed up as skips on my setup as I haven't tried to
> enable them here yet.

Yes, these new tests run on my system without my doing anything to 
enable them, and the mmc_info test fails. In at least one case, the new 
test runs because it's using the configuration that was intended for the 
mmc_read test.