Re: [Fuego] [PATCH 1/2] Kernel_build: Update to parse and get test_build time

Sireesha,

Before applying this, I have a few questions and comments.
See inline below for some of these, but first...

what is the rationale for wanting to add the build duration as a measure (o=
r metric)
to the run.json file?  Is it to make this an element of the pass criteria f=
or the test
(thus turning this into a kind of benchmark test) or is it just for an extr=
a display
element for this test. =20

I don't have a big problem with this, but it is unusual.  Of course, the bu=
ild_kernel
test itself is a bit unusual.

Normally, a functional test will have one or more testcases that result in =
PASS/FAIL
and possibly it may have a criteria.json file to indicate that some FAIL re=
sults are allowed.
Usually, there are no measures in a functional test's run.json file (and no=
 graphs in
the functional test's output).  Benchmark tests, on the other hand, have on=
e or more
measures in their output, which need to be parsed, and evaluated against so=
me reference
value.

The code, as submitted, adds support for parsing the build duration for the=
 kernel build.
However, the patch does not change the test_processing() function in fuego_=
test.sh.  So there's
no change of this code into a benchmarking function.  It still passes or fa=
ils based on
the result of searching the build log (which, because of the use of 'log_th=
is' in the test_build()
function (during the build phase for this test), becomes part of the test l=
og for the
test.

This test (the current Functional.kernel_build) is meant to check that the =
code compiles at all,
not to measure how fast it compiles.  And the way the fuego_test.sh is stru=
ctured,
the test runs the compilation on the Fuego host, not on the board.  So the =
resulting
metric for the test duration would not normally provide any information abo=
ut
the speed of the board.  This might be confusing in your case, as I believe=
 Toshiba
runs tests on the 'local' board, where the Fuego host is the same as the ta=
rget board
(the device under test).

If you want a test that measures how long a board takes to perform a compil=
ation
of the kernel, then this should be structured as a new Benchmark test, and
the actual compilation should be placed in a fuego_test.sh test_run() funct=
ion,
rather than in the test_build() function.  Arguably, that is how this test =
should have
been structured also, but when this was written we were experimenting with
using the fuego_test.sh test_deploy() function to provision the kernel imag=
e onto
the board, and it was envisioned to possibly to a second testcase inside te=
st_run
to validate that the new kernel booted on the board.  That would have made =
this
into both a build and a boot test, but that second testcase was never imple=
mented.

In any event, please let me know your reason for doing this work.  If you w=
ant a
benchmark of kernel compilation speed on a board, it would be preferable to
write a new Benchmark test structured a bit differently than this.  If you =
just want
an extra piece of data included in the run.json file and displayed as part
of the Jenkins interface for jobs for this test, then this might be fine.

> -----Original Message-----
> From: [email protected] <[email protected]=
m>
>=20
> From: sireesha <[email protected]>
>=20
> scripts/parser/common.py: Update reference to 'Consolelog' to parse build=
 time
>=20
> chart_config.json: Add reference to get test build time
>=20
> parser.py: Implement to parse test build time from console log
>=20
> reference.json: Add reference for test build time
>=20
> test.yaml: Add data files related to kernel build
>=20
> Signed-off-by: sireesha <[email protected]>
> ---
>  scripts/parser/common.py                      |  1 +
>  .../Functional.kernel_build/chart_config.json |  5 +++++
>  tests/Functional.kernel_build/parser.py       | 21 +++++++++++++++++++
>  tests/Functional.kernel_build/reference.json  | 20 ++++++++++++++++++
>  tests/Functional.kernel_build/test.yaml       |  3 +++
>  5 files changed, 50 insertions(+)
>  create mode 100644 tests/Functional.kernel_build/chart_config.json
>  create mode 100644 tests/Functional.kernel_build/parser.py
>  create mode 100644 tests/Functional.kernel_build/reference.json
>=20
> diff --git a/scripts/parser/common.py b/scripts/parser/common.py
> index d7dcfbd..ba28cc1 100644
> --- a/scripts/parser/common.py
> +++ b/scripts/parser/common.py
> @@ -114,6 +114,7 @@ for env_var in env_list:
>  REF_JSON  =3D TEST_HOME + '/reference.json'
>  CHART_CONFIG_JSON  =3D TEST_HOME + '/chart_config.json'
>  TEST_LOG =3D '%s/logs/%s/%s.%s.%s.%s/testlog.txt' % (FUEGO_RW, TESTDIR, =
NODE_NAME, TESTSPEC, BUILD_NUMBER, BUILD_ID)
> +CONSOLE_LOG =3D '%s/logs/%s/%s.%s.%s.%s/consolelog.txt' % (FUEGO_RW, TES=
TDIR, NODE_NAME, TESTSPEC, BUILD_NUMBER,
> BUILD_ID)
This seems OK.

>  RUN_JSON =3D LOGDIR + '/run.json'
>=20
>  #Here are some pre-packaged regex strings
> diff --git a/tests/Functional.kernel_build/chart_config.json b/tests/Func=
tional.kernel_build/chart_config.json
> new file mode 100644
> index 0000000..a8802de
> --- /dev/null
> +++ b/tests/Functional.kernel_build/chart_config.json
> @@ -0,0 +1,5 @@
> +{
> +    "chart_type": "measure_plot",
> +    "measures": ["default.test_build"]
I'd rather this was "build_duration", instead of "test_build".

I think this generates confusion between:
 1) the name of the test "kernel_build"
 2) the name of the function "test_build"
 3) the name of the metric or measure "build_duration"

> +}
> +
> diff --git a/tests/Functional.kernel_build/parser.py b/tests/Functional.k=
ernel_build/parser.py
> new file mode 100644
> index 0000000..78f35c3
> --- /dev/null
> +++ b/tests/Functional.kernel_build/parser.py
> @@ -0,0 +1,21 @@
> +#!/usr/bin/python3
> +import os, re, sys
> +import common as plib
> +
> +cur_search_str =3D "^(Fuego.test_build.duration=3D)\ *([\d]{1,8}.[\d]{1,=
4}).*"

The use of periods instead of spaces in the first part of this regex is con=
fusing.
You are trying to match an exact string that Fuego outputs as part of the=20
test console log.  This will work, but 'Fuego test_build duration' is more =
specific
and IMHO accurate, for this search string.  (That is, use a single space in=
stead of
the '.', for this first group.

> +
> +print("Reading current values from " + plib.CONSOLE_LOG)
> +cur_file =3D open(plib.CONSOLE_LOG,'r')
> +
> +lines =3D cur_file.readlines()
> +cur_file.close()
> +
> +measurements =3D {}
> +for line in lines:
> +    m =3D re.match(cur_search_str, line)
> +    if m:
> +        value =3D m.group(2)
> +        measurements['default.kernel_build'] =3D [{"name": "test_build",=
 "measure" : value}]
I'd prefer if the measure was called 'build_duration', instead of 'test_bui=
ld'.

> +
> +sys.exit(plib.process(measurements))
> +
> diff --git a/tests/Functional.kernel_build/reference.json b/tests/Functio=
nal.kernel_build/reference.json
> new file mode 100644
> index 0000000..9d8adfc
> --- /dev/null
> +++ b/tests/Functional.kernel_build/reference.json
> @@ -0,0 +1,20 @@
> +{
> +    "test_sets":[
> +        {
> +            "name":"default",
> +            "test_cases":[
> +                {
> +                    "name":"kernel_build",
> +                    "measurements":[
> +                        {
> +                            "name":"test_build",
I'd prefer if the measure was called 'build_duration', instead of 'test_bui=
ld'.

> +                            "unit":"s"
> +                        }
> +
> +                    ]
> +                }
> +            ]
> +        }
> +    ]
> +}
> +
> diff --git a/tests/Functional.kernel_build/test.yaml b/tests/Functional.k=
ernel_build/test.yaml
> index c076bd7..304218e 100644
> --- a/tests/Functional.kernel_build/test.yaml
> +++ b/tests/Functional.kernel_build/test.yaml
> @@ -46,3 +46,6 @@ data_files:
>      - fuego_test.sh
>      - spec.json
>      - test.yaml
> +    - parser.py
> +    - reference.json
> +    - chart_config.json
> --
> 2.20.1
>=20

This displays a good knowledge of how Fuego works.  Thanks for the contribu=
tion.

Please answer the questions above, and we can work out the best way to supp=
ort
what you want.
 -- Tim