[OE-core] [PATCH] resulttool/merge: Enable merge results to one file

[Yeoh, Ee Peng]

Hi RP,

Yes, you are right, I missed the current feature to merge to flat file. Thanks for your inputs and sharing. 

After we tested the merge to flat file features, these are our findings. 
1) Existing merge could not support use case where both base and target are directory. 
Traceback (most recent call last):
  File "/home/poky/scripts/resulttool", line 93, in <module>
    sys.exit(main())
  File "/home/poky/scripts/resulttool", line 87, in main
    ret = args.func(args, logger)
  File "/home/poky/scripts/lib/resulttool/merge.py", line 22, in merge
    resultutils.append_resultsdata(results, args.base_results, configmap=resultutils.store_map)
  File "/home/poky/scripts/lib/resulttool/resultutils.py", line 47, in append_resultsdata
    with open(f, "r") as filedata:
IsADirectoryError: [Errno 21] Is a directory: '<path_to_base_results>'

2) Existing merge will always create testseries configuration where this configuration has important implication to report and regression. 

For current QA process, we need the merge to be more flexible where it will allow merge where both base and target are directory, furthermore we need control over when we want to create the testseries configuration. 

To fulfill both of these requirements, we tried the below: 

Previously the append_resultsdata function only allow append the results data to the map_results data (where map_results data wrapped the results data with configuration map as the key).
 
Initially, we tried to implement an extra function where it will enable append one map_results to another map_results data. But we abandoned this alternative as this new append function will be pretty much a duplicated function to the original append_resultsdata, and these will create two append functions which they might be both hard to maintain and confusing. Thus, we tried to refactor the append function to enable a single append function to be used  for most of the situation. Furthermore, since the map_results were
only needed by report and regression, we pulled the instructions used to turn results data to map_results data to another function. Finally, we renamed the functions and arguments to clearly seperated the functions using results data from the one using map_results data.

The new patches are at below:
http://lists.openembedded.org/pipermail/openembedded-core/2019-March/280547.html
http://lists.openembedded.org/pipermail/openembedded-core/2019-March/280548.html

Please let us know if you have any more inputs or questions. Thank you very much for your attention and help!

Best regards,
Yeoh Ee Peng 

-----Original Message-----
From: Richard Purdie [mailto:richard.purdie at linuxfoundation.org] 
Sent: Tuesday, March 26, 2019 8:52 PM
To: Yeoh, Ee Peng <ee.peng.yeoh at intel.com>; openembedded-core at lists.openembedded.org
Subject: Re: [OE-core] [PATCH] resulttool/merge: Enable merge results to one file

On Tue, 2019-03-26 at 10:02 +0800, Yeoh Ee Peng wrote:
> QA team execute extra testing that create multiple test result files, 
> where these test result files need to be merged into a single file 
> under certain use case.
> 
> Enable merge to allow merging results into a single test result file.
> 
> Signed-off-by: Yeoh Ee Peng <ee.peng.yeoh at intel.com>
> ---
>  scripts/lib/resulttool/merge.py       | 29 ++++++++++++-
>  scripts/lib/resulttool/resultutils.py | 76 
> +++++++++++++++++++++++++----------
>  2 files changed, 82 insertions(+), 23 deletions(-)
> 
> diff --git a/scripts/lib/resulttool/merge.py 
> b/scripts/lib/resulttool/merge.py index 3e4b7a3..90b3cb3 100644
> --- a/scripts/lib/resulttool/merge.py
> +++ b/scripts/lib/resulttool/merge.py
> @@ -17,6 +17,26 @@ import json
>  import resulttool.resultutils as resultutils
>  
>  def merge(args, logger):
> +    if args.merge_to_one:
> +        if os.path.isdir(args.target_results):
> +            target_results = resultutils.load_results(args.target_results)
> +        else:
> +            target_results = resultutils.append_results({}, 
> + args.target_results)

Looking at load_resultsdata:

def load_resultsdata(source, configmap=store_map):
    results = {}
    if os.path.isfile(source):
        append_resultsdata(results, source, configmap)
        return results

The code above can therefore be simplified to:

target_results = resultutils.load_results(args.target_results)

?

> +        if os.path.isdir(args.base_results):
> +            base_results = resultutils.load_results(args.base_results)
> +            results = resultutils.append_results(target_results, base_results)
> +        else:
> +            results = resultutils.append_results(target_results, 
> + args.base_results)


Again, I'm not sure you need to differentiate between a file and a directory given the way the code works internally?

> +
> +        target_file_dir = os.path.join(os.path.dirname(args.target_results), 'merged_results/testresults.json')
> +        if os.path.isdir(args.target_results):
> +            target_file_dir = os.path.join(args.target_results, 'merged_results/testresults.json')
> +        if args.merge_to_one_dir:
> +            target_file_dir = os.path.join(args.merge_to_one_dir, 'testresults.json')
> +        resultutils.make_directory_and_write_json_file(target_file_dir, results)
> +        logger.info('Merged results to %s' % target_file_dir)
> +        return 0

Isn't this similar to calling load_resultsdata with the 'flatten'
mapping, i.e. resultutils.flatten_map?

That would match the code already in the merge command:

        results = resultutils.load_resultsdata(args.base_results, configmap=resultutils.flatten_map)
        if os.path.exists(args.target_results):
            resultutils.append_resultsdata(results, args.target_results, configmap=resultutils.flatten_map)
        resultutils.save_resultsdata(results, os.path.dirname(args.target_results), fn=os.path.basename(args.target_results))

i.e. if you call:

"resulttool merge XXXX merged_results/testresults.json"

it should merge the results from XXXX into the results file specified?

In other words, doesn't the tool already support this?

Cheers,

Richard