[U-Boot] [PATCH] test/py: hush_if_test: Add tests to cover octal/hex values

[Michal Simek <michal.simek@xilinx.com>]

On 22. 10. 19 17:54, Stephen Warren wrote:
> On 10/21/19 5:46 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, 21 Oct 2019 at 17:04, Stephen Warren <swarren@wwwdotorg.org>
>> wrote:
>>>
>>> On 10/21/19 4:53 PM, Simon Glass wrote:
>>>> Hi Michal,
>>>>
>>>> On Tue, 15 Oct 2019 at 00:09, Michal Simek <michal.simek@xilinx.com>
>>>> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On 11. 10. 19 17:53, Simon Glass wrote:
>>>>>> Hi Michal,
>>>>>>
>>>>>> On Fri, 11 Oct 2019 at 01:50, Michal Simek
>>>>>> <michal.simek@xilinx.com> wrote:
>>>>>>>
>>>>>>> On 10. 10. 19 19:06, Simon Glass wrote:
>>>>>>>> Hi Michal,
>>>>>>>>
>>>>>>>> On Thu, 10 Oct 2019 at 05:44, Michal Simek
>>>>>>>> <michal.simek@xilinx.com> wrote:
>>>>>>>>>
>>>>>>>>> Extend test suite to cover also automatic octal/hex
>>>>>>>>> converstions which
>>>>>>>>> haven't been implemented in past.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Depends on
>>>>>>>>> https://lists.denx.de/pipermail/u-boot/2019-September/383309.html
>>>>>>>>>
>>>>>>>>> There are of course other tests which we can run but not sure
>>>>>>>>> if make sense
>>>>>>>>> to have there all combinations. The most interesting are mixed
>>>>>>>>> tests which
>>>>>>>>> are failing before patch above is applied.
>>>>>>>>> Definitely please let me know if you want to add any other test.
>>>>>>>>> ---
>>>>>>>>>    test/py/tests/test_hush_if_test.py | 27
>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>    1 file changed, 27 insertions(+)
>>>>>>>>>
>>>>>>>>
>>>>>>>> I worry that these tests might be very slow since it requires a
>>>>>>>> lot of
>>>>>>>> interaction with U-Boot over a pipe. Is it possible to put them
>>>>>>>> in C
>>>>>>>> code instead, e.g. cmd_ut?
>>>>>>>
>>>>>>> I have of course running it on my HW and it is quite fast. It is
>>>>>>> just 16
>>>>>>> more simple tests. And if this breaks gitlab/travis CI loops then we
>>>>>>> have bigger problem.
>>>>>>
>>>>>> I mean running these tests on sandbox. The interactions with the
>>>>>> sandbox command line are quite slow I think.
>>>>>
>>>>>
>>>>> I am not sharing this concern.
>>>>>
>>>>> Before:
>>>>> [u-boot]$ time ./test/py/test.py --bd sandbox -s -k hush >/dev/null
>>>>>
>>>>> real    0m2,403s
>>>>> user    0m1,263s
>>>>> sys     0m0,299s
>>>>>
>>>>> After
>>>>> [u-boot]$ time ./test/py/test.py --bd sandbox -s -k hush >/dev/null
>>>>>
>>>>> real    0m2,864s
>>>>> user    0m1,563s
>>>>> sys     0m0,305s
>>>>>
>>>>> And if 0.4s on testing will cause issues somewhere else we have
>>>>> different kind of problem.
>>>>
>>>> +Stephen Warren
>>>>
>>>> I originally mentioned this concern to Stephen we the test setup was
>>>> created. At present even 'make qcheck' takes over a minute. Adding
>>>> half a second to this every time we add a new test is not going to
>>>> lead to a good place.
>>>>
>>>> Stephen made some improvements to speed things up, and suggested that
>>>> the problem would not bear out. The alternative was presumably to
>>>> build U-Boot into a Python module to avoid the comms overhead. But we
>>>> didn't go that path.
>>>>
>>>> So I think we should only use Python when the tests cannot be
>>>> written in C.
>>>
>>> I don't really see any concern with the addition of a couple extra
>>> seconds of test. Clearly I'd rather see the test written in Python and
>>> using external interfaces (i.e. the shell) where they test features
>>> accessible through those interfaces, since that allows them to be
>>> validated on all platforms, rather than only in sandbox. I feel that
>>
>> But cmd_ut.c works fine on non-sandbox platforms. I'm asking that we
>> do the same approach here.
>>
>> We can use run_command() and run_command_list()
> 
> In that case, I'd agree it's fine to use that approach since presumably
> those functions run through the standard shell parsing code. But I still
> wouldn't want to prevent anyone from invoking stuff from test/py myself,
> even if you might:-)

Ok. Would be good to get any outcome of this discussion regarding this
one patch. Right now this is what I have and I have tested it. The patch
which adds this functionality is already in the tree.

I expect that the same logic can be applied to all tests in this file
that's why I think that would be the best to add TODO to this file to
let everybody know what should happen with these tests and how it should
be converted.

Thanks,
Michal