[Fuego] [PATCH 1/9] chart: add log links for the tests which have separated log file

[Liu, Wenlong]

> -----Original Message-----
> From: Tim.Bird at sony.com [mailto:Tim.Bird at sony.com]
> Sent: Thursday, February 01, 2018 9:44 AM
> To: Liu, Wenlong/刘 文龙 <liuwl.fnst at cn.fujitsu.com>;
> fuego at lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 1/9] chart: add log links for the tests which
> have separated log file
> 
> > -----Original Message-----
> > From: Liu Wenlong
> >
> > Add log links for the tests which have separated log file, Such as,
> > Functional.LTP in the current Fuego.
> >
> > The separated log link can help us to find the error log easily.
> > This feature indeed helped me a lot.
> >
> > I also added link to "testlog.txt" for those tests who don't have the
> > separated log.(This might be a little unnecessary)
> >
> > Signed-off-by: Liu Wenlong <liuwl.fnst at cn.fujitsu.com>
> > ---
> >  engine/scripts/parser/prepare_chart_data.py | 33
> > +++++++++++++++++++++++------
> >  1 file changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/engine/scripts/parser/prepare_chart_data.py
> > b/engine/scripts/parser/prepare_chart_data.py
> > index af5f6d3..ffb042b 100644
> > --- a/engine/scripts/parser/prepare_chart_data.py
> > +++ b/engine/scripts/parser/prepare_chart_data.py
> > @@ -381,6 +381,8 @@ def cmp_alpha_num(a, b):
> >  def make_testcase_table(test_name, chart_config, entries):
> >      # make a table of testcase results for every testcase
> >      chart_list = []
> > +    jenkins_web_prefix = "/fuego"
> I think we can probably use JENKINS_URL instead of this.
> 
> > +    jenkins_root_path = "/var/lib/jenkins/"
> 
> I'd rather we looked for these logs via the log directory, than the jenkins
> directory.

OK, I will change it.

> >
> >      # get a list of (board,test sets) in the data
> >      # FIXTHIS - use list of test sets in chart_config, if present @@
> > -421,14 +423,32 @@ def make_testcase_table(test_name, chart_config,
> > entries):
> >                  dprint("making a new row for '%s'" % row_key)
> >                  result_map[row_key] = {}
> >
> > +            # entry.tguid: e.g. math.abs01.
> > +            tguid_parts = entry.tguid.split(".")
> > +            tguid_testset = ".".join(tguid_parts[:-1])
> > +            tguid_testcase = tguid_parts[-1]
> > +
> > +            # get the name that contains board, spec, build number. E.g.
> > porter.default.1.1
> > +            log_bts_name = '%s.%s.%s.%s' % (entry.board, entry.spec,
> > str(entry.build_number), str(entry.build_number))
> > +            # separated log files, e.g.
> >
> /var/lib/jenkins/userContent/fuego.logs/Functional.LTP/ubuntu.math.7.7
> > /re
> > sult/math/outputs/abs01.log
> This is relative to the URL home, not /var/lib/jenkins, for purposes of
> this code.  This comment is misleading.
> 
> > +            log_file =
> > '/userContent/fuego.logs/%s/%s/result/%s/outputs/%s.log' %
> > (entry.testname, log_bts_name, tguid_testset, tguid_testcase)
> > +            # testlog files, e.g.
> >
> /fuego/userContent/fuego.logs/Functional.croco/porter.default.${BUILD_
> > N
> > UMBER}.${BUILD_ID}/testlog.txt
> > +            testlog_file =
> > + '/userContent/fuego.logs/%s/%s/testlog.txt' %
> > (entry.testname, log_bts_name)
> > +
> > +            # check if the separated log path exist
> > +            if os.path.exists(jenkins_root_path + log_file):
> > +                entry.result = '<a href=\"%s%s\">%s</a>' %
> > + (jenkins_web_prefix,
> > log_file, entry.result)
> > +            elif os.path.exists(jenkins_root_path + testlog_file):
> > +                entry.result = '<a href=\"%s%s\">%s</a>' %
> > + (jenkins_web_prefix,
> > testlog_file, entry.result)
> > +
>
> >              # add a data point (result) for this entry
> >              result_map[row_key][entry.build_number] = entry.result
> >              # count the result
> > -            if entry.result=="PASS":
> > +            if "PASS" in entry.result:
> I'm not sure why this is needed.   The result in an entry in this code
> should always be exactly one of FAIL, PASS, SKIP or ERROR.
> 
> Using python's 'in' shouldn't be needed, and is a more expensive
> operation.   Did you see the use of '==' for this comparison fail
> somewhere?  If so, that's a bug we should track down.

Emm, sorry, I changed the result in an entry above.
It's better to use a temporary variable to store this value which contains URL info.
And then, "==" will be ok here.

I will improve those patches together after your review.

> (The same comment applies to the rest of these conversions.)
> 
> >                  build_num_map[entry.build_number][0] += 1
> > -            elif entry.result=="FAIL":
> > +            elif "FAIL" in entry.result:
> >                  build_num_map[entry.build_number][1] += 1
> > -            elif entry.result=="SKIP":
> > +            elif "SKIP" in entry.result:
> >                  build_num_map[entry.build_number][2] += 1
> >              else:
> >                  build_num_map[entry.build_number][3] += 1 @@ -475,12
> > +495,13 @@ def make_testcase_table(test_name, chart_config,
> > entries):
> >                      value = result_map[tc][bn]
> >                  except:
> >                      value = ""
> > -                if value=="PASS":
> > +                if "PASS" in value:

But for this "value" here, it contains the the URL info and not aways be the one of FAIL, PASS, SKIP or ERROR.

> >                      cell_attr = 'style="background-color:#ccffcc"'
> > -                elif value=="FAIL":
> > +                elif "FAIL" in value:
> >                      cell_attr = 'style="background-color:#ffcccc"'
> >                  else:
> > -                    cell_attr = ''
> > +                    cell_attr = 'align="center"'
> > +                    value='-'
> >                  row += ("<td %s>" % cell_attr) + value + "</td>"
> >              row += '</tr>'
> >              html += row
> > --
> > 2.7.4
> 
> Thanks.
>  -- Tim
>