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

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

[Michal Simek]

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(+)

diff --git a/test/py/tests/test_hush_if_test.py b/test/py/tests/test_hush_if_test.py
index bba8d41d9648..252e8d1521ed 100644
--- a/test/py/tests/test_hush_if_test.py
+++ b/test/py/tests/test_hush_if_test.py
@@ -52,6 +52,33 @@ subtests = (
     ('test 123 -ge 123', True),
     ('test 123 -ge 456', False),
 
+    # Octal tests
+
+    ('test 010 -eq 010', True),
+    ('test 010 -eq 011', False),
+
+    ('test 010 -ne 011', True),
+    ('test 010 -ne 010', False),
+
+    # Hexadecimal tests
+
+    ('test 0x2000000 -gt 0x2000001', False),
+    ('test 0x2000000 -gt 0x2000000', False),
+    ('test 0x2000000 -gt 0x1ffffff', True),
+
+    # Mixed tests
+
+    ('test 010 -eq 10', False),
+    ('test 010 -ne 10', True),
+    ('test 0xa -eq 10', True),
+    ('test 0xa -eq 012', True),
+
+    ('test 2000000 -gt 0x1ffffff', False),
+    ('test 0x2000000 -gt 1ffffff', True),
+    ('test 0x2000000 -lt 1ffffff', False),
+    ('test 0x2000000 -eq 2000000', False),
+    ('test 0x2000000 -ne 2000000', True),
+
     ('test -z ""', True),
     ('test -z "aaa"', False),
 
-- 
2.17.1

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

[Stephen Warren]

On 10/10/19 5:44 AM, Michal Simek wrote:
> Extend test suite to cover also automatic octal/hex converstions which
> haven't been implemented in past.

Acked-by: Stephen Warren <swarren@nvidia.com>

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

[Simon Goldschmidt]

Stephen Warren <swarren@wwwdotorg.org> schrieb am Do., 10. Okt. 2019, 17:56:

> On 10/10/19 5:44 AM, Michal Simek wrote:
> > Extend test suite to cover also automatic octal/hex converstions which
> > haven't been implemented in past.
>
> Acked-by: Stephen Warren <swarren@nvidia.com>
>

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

>

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

[Simon Glass]

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?

Regards,
Simon

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

[Michal Simek]

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 see that CONFIG_UNIT_TEST is enabled only for sandbox. Do it make
sense to enable it also for others to have better coverage?

Thanks,
Michal

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

[Simon Glass]

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 see that CONFIG_UNIT_TEST is enabled only for sandbox. Do it make
> sense to enable it also for others to have better coverage?

Hopefully the sandbox tests cover it. We are not necessarily trying to
find compiler bugs, just make sure that code has test coverage.

Regards,
Simon

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

[Michal Simek]

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.

Thanks,
Michal

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

[Simon Glass]

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.

Regards,
Simon

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

[Stephen Warren]

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 
sandbox is good for unit tests and providing host-based smoke tests, but 
nothing beats actually being able to test real platforms.

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

[Simon Glass]

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()


> sandbox is good for unit tests and providing host-based smoke tests, but
> nothing beats actually being able to test real platforms.

Agreed, but I think that is a separate issue.

Regards,
Simon

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

[Stephen Warren]

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:-)

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

[Michal Simek]

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

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

[Simon Glass]

Hi Michal,

On Thu, 24 Oct 2019 at 00:51, Michal Simek <michal.simek@xilinx.com> wrote:
>
> 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.

Yes please add a TODO, but I don't want to add more tests here for
someone to convert!

I just timed it - 2.5 seconds to run the hush tests, 22 seconds to run
the whole test suite - so these tests take 10% of the time! As you
mention they run instantly when done in C.

I run these tests a lot - the total time for 'make qcheck' is about
75s for me and I run it a lot, so am actively looking to reduce this
down.

We may need to rethink the pytest stuff a bit, perhaps building U-Boot
as a Python library? But in any case, please add a C test. It is
really easy to do, e.g. like cmd_ut.c

Regards,
Simon