[PATCH] D114770: [LNT] Combine perf data metrics from several files

[Chris Matthews via Phabricator via llvm-commits]

cmatthews requested changes to this revision.
cmatthews added inline comments.
This revision now requires changes to proceed.


================
Comment at: lnt/testing/profile/perf.py:16
 
+def mergeRecursively(dct1, dct2):
+    for k, v in dct2.items():
----------------
It would be nice to add a type annotation here, so we can find issues before hitting "assert False" in production. 


================
Comment at: lnt/testing/profile/perf.py:16
 
+def mergeRecursively(dct1, dct2):
+    for k, v in dct2.items():
----------------
cmatthews wrote:
> It would be nice to add a type annotation here, so we can find issues before hitting "assert False" in production. 
I think this should be "merge_recursivly", to be pep8 compliant. 

Does this pass flake8? 

Can you add a doc comment.

Can you add a small unit test for this.


================
Comment at: lnt/testing/profile/perf.py:24
+            else:
+                assert False
+        else:
----------------
I don't like assert False, especially with no extra logging. It will be hard to debug if we hit in production. How about raise a ValueError or TypeError or NotImplementedYet, and in the message. mention the types responsible for the error ?


================
Comment at: lnt/testing/profile/perf.py:48
+            data = {}
+            for fname in glob.glob("%s*" % f):
+                cur_data = cPerf.importPerf(fname, objdump, binaryCacheRoot)
----------------
This functionality needs a unit test. 




================
Comment at: lnt/testing/profile/perf.py:48
+            data = {}
+            for fname in glob.glob("%s*" % f):
+                cur_data = cPerf.importPerf(fname, objdump, binaryCacheRoot)
----------------
cmatthews wrote:
> This functionality needs a unit test. 
> 
> 
Could this glob be any more specific?


Repository:
  rLNT LNT

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114770/new/

https://reviews.llvm.org/D114770