[OE-core] [PATCH] resulttool/merge: Merge files from folders and control add testseries

[Yeoh, Ee Peng]

Hi RP,

Yes, we will separate the changes into different patches as suggested.
Thank you for your inputs. 

Thanks,
Ee Peng 

-----Original Message-----
From: Richard Purdie [mailto:richard.purdie at linuxfoundation.org] 
Sent: Thursday, March 28, 2019 3:43 PM
To: Yeoh, Ee Peng <ee.peng.yeoh at intel.com>; openembedded-core at lists.openembedded.org
Subject: Re: [OE-core] [PATCH] resulttool/merge: Merge files from folders and control add testseries

Hi Ee Peng,

This patch isn't really in a form where it can be easily reviewed or accepted. A given patch needs to do one specific thing, so for example if you're renaming/refactoring a function that would belong in its own patch. If you're changing functionality, that would also be best in its own patch.

In the patch below you make various refactoring changes as well as making functionality changes meaning its very hard to separate the real functionality change from the rest of the "noise" of the renaming and refactoring. This makes review extremely difficult.

I'm not even sure I agree with some of the renaming/refactoring, e.g.
make_directory_and_write_json_file is a horrible name for a function and it only appears to be called once? There are probably other issues but its really hard to tell.

Could you split up this series, ideally showing the functionality change in its own patch please. I'm not promising it would all be accepted but it would at least allow review and allow the changes to be understood.

Cheers,

Richard

On Thu, 2019-03-28 at 12:54 +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 under various use 
> cases.
> Furthermore, during results merging, user need control over the 
> testseries configuration creation as this configuration has important 
> implication to report and regression.
> 
> Current merge do not support merge results 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>'
> 
> This patches enable merge for both base and target as directory.
> Also, enable control the creation of testseries configuration.
> 
> 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 in all the 
> situation. Futhermore, 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.
> 
> Signed-off-by: Yeoh Ee Peng <ee.peng.yeoh at intel.com>
>