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

Please find below inline comments

> -----Original Message-----
> From: Bird, Tim <[email protected]>
> Sent: Friday, April 15, 2022 2:21 AM
> To: nakkala sireesha(=1B$B#T#S#I#P=1B(B) <[email protected]=
om>
> Cc: [email protected]; dinesh kumar(=1B$B#T#S#I#P=1B(B)
> <[email protected]>; hayashi kazuhiro(=1B$BNS=1B(B =1B$BOB9(=
=1B(B =1B$B""#S#W#C"~#A#C=1B(B
> =1B$B#T=1B(B) <[email protected]>
> Subject: RE: [PATCH 1/2] Kernel_build: Update to parse and get test_build
> time
>=20
> Sireesha,
>=20
> Before applying this, I have a few questions and comments.
> See inline below for some of these, but first...
>=20
> what is the rationale for wanting to add the build duration as a measure =
(or
> metric) to the run.json file?  Is it to make this an element of the pass =
criteria
> for the test (thus turning this into a kind of benchmark test) or is it j=
ust for an
> extra display element for this test.
>=20
> I don't have a big problem with this, but it is unusual.  Of course, the
> build_kernel test itself is a bit unusual.
>=20
> Normally, a functional test will have one or more testcases that result i=
n
> PASS/FAIL and possibly it may have a criteria.json file to indicate that =
some
> FAIL results 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 h=
and,
> have one or more measures in their output, which need to be parsed, and
> evaluated against some reference value.
>=20
> The code, as submitted, adds support for parsing the build duration for t=
he
> 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 fails based on the result of searching the =
build log
> (which, because of the use of 'log_this' in the test_build() function (du=
ring
> the build phase for this test), becomes part of the test log for the test=
.
>=20
> This test (the current Functional.kernel_build) is meant to check that th=
e
> code compiles at all, not to measure how fast it compiles.  And the way t=
he
> fuego_test.sh is structured, the test runs the compilation on the Fuego h=
ost,
> not on the board.  So the resulting metric for the test duration would no=
t
> normally provide any information about the speed of the board.  This migh=
t
> be confusing in your case, as I believe Toshiba runs tests on the 'local'=
 board,
> where the Fuego host is the same as the target board (the device under te=
st).
>=20
> If you want a test that measures how long a board takes to perform a
> compilation 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() function, rather than in the test_build() functi=
on.
> Arguably, that is how this test should have been structured also, but whe=
n
> this was written we were experimenting with using the fuego_test.sh
> test_deploy() function to provision the kernel image onto the board, and =
it
> was envisioned to possibly to a second testcase inside test_run to valida=
te
> 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
> implemented.
>=20
> In any event, please let me know your reason for doing this work.  If you
> want a benchmark of kernel compilation speed on a board, it would be
> preferable to write a new Benchmark test structured a bit differently tha=
n
> this.  If you just want an extra piece of data included in the run.json f=
ile and
> displayed as part of the Jenkins interface for jobs for this test, then t=
his might
> be fine.
>=20
Thank you for explaining us in detail.

Actually we want to run this on board to measure the Kernel build time,
but in our case both host and board are same so the below changes are worke=
d for us.
To apply this change in upstream I think now it need to change it to Benchm=
ark test.
We are not familiar to create  a new Benchmark test in fuego-core, so curre=
ntly please ignore this change.

Will come back to you if  we need to create a Benchmark kernel build test.

> > -----Original Message-----
> > From: [email protected]
> > <[email protected]>
> >
> > From: sireesha <[email protected]>
> >
> > scripts/parser/common.py: Update reference to 'Consolelog' to parse
> > build time
> >
> > chart_config.json: Add reference to get test build time
> >
> > parser.py: Implement to parse test build time from console log
> >
> > reference.json: Add reference for test build time
> >
> > test.yaml: Add data files related to kernel build
> >
> > 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
> >
> > 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,
> > +TESTDIR, NODE_NAME, TESTSPEC, BUILD_NUMBER,
> > BUILD_ID)
> This seems OK.
>=20
> >  RUN_JSON =3D LOGDIR + '/run.json'
> >
> >  #Here are some pre-packaged regex strings diff --git
> > a/tests/Functional.kernel_build/chart_config.json
> > b/tests/Functional.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".
>=20
> 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"
>=20
> > +}
> > +
> > diff --git a/tests/Functional.kernel_build/parser.py
> > b/tests/Functional.kernel_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}).*"
>=20
> The use of periods instead of spaces in the first part of this regex is
> confusing.
> You are trying to match an exact string that Fuego outputs as part of the=
 test
> console log.  This will work, but 'Fuego test_build duration' is more spe=
cific
> and IMHO accurate, for this search string.  (That is, use a single space =
instead
> of the '.', for this first group.
>=20
> > +
> > +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_b=
uild'.
>=20
> > +
> > +sys.exit(plib.process(measurements))
> > +
> > diff --git a/tests/Functional.kernel_build/reference.json
> > b/tests/Functional.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_b=
uild'.
>=20
> > +                            "unit":"s"
> > +                        }
> > +
> > +                    ]
> > +                }
> > +            ]
> > +        }
> > +    ]
> > +}
> > +
> > diff --git a/tests/Functional.kernel_build/test.yaml
> > b/tests/Functional.kernel_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
> contribution.
>=20
> Please answer the questions above, and we can work out the best way to
> support what you want.
>  -- Tim