[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 at 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 at 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 at 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 at 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 at 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 at 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