[PATCH] D43314: [lit] - Allow 1 test to report multiple micro-test results to provide support for microbenchmarks.

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Looks nearly good to me. I'd just opt for a different naming of the sub-tests. Also some nitpicks/comments on other nitpicks below:



================
Comment at: utils/lit/lit/main.py:86
+        if test.result.microResults:
+            items = sorted(test.result.microResults.items())
+            for micro_test_name, micro_test in items:
----------------
rengolin wrote:
> IIRC Python 2.x and 3.x behave different regarding `.items()`.
> 
> Do we force Python 3 for LIT?
`.items()` use should be fine here (it'll give you a full copy in python2 and a lazy iterator in python3. But except for higher memory they should behave the same, and this doesn't look like we care about the memory usage of a few entries).


================
Comment at: utils/lit/lit/main.py:134
+                # Add micro test name before test extension
+                rex = re.compile("(.+?)(\.[^.]*$|$)")
+                result = rex.match(parent_name)
----------------
Appending the subtest name before .test feels odd to me as there is no actual file with the name `testname/microtest.test` how about `testname.test:microtest` instead?


================
Comment at: utils/lit/tests/test-data-micro.py:14
+# CHECK-NEXT: ***
+# CHECK-NEXT: *** MICRO-TEST: test{{[0-2]}}
+# CHECK-NEXT: micro_value0: 4
----------------
rengolin wrote:
> Both micro-test and micro-results are being stored in dictionaries, that means their order is not guaranteed.
> 
> I assume that's the reason for the `{{[0-2]}}` regex here, but a similar solution would be needed for the values as well.
> 
> I recommend you look at CHECK-DAG:
> 
>     # CHECK-NEXT: *** MICRO-TEST: test{{[0-2]}}
>     # CHECK-DAG: micro_value0: 4
>     ​# CHECK-DAG: micro_value1: 1.3
> 
> This means there are two lines after `MICRO-TEST` which have those two values, in any order.
> 
> It could match to a list after the next `MICRO-TEST`, however, if you have three `CHECK-NEXT:` of `MICRO-TEST`, then the last one would fail if you match the `CHECK-DAG` on the wrong line, so you're covered.
> 
> Just repeat that pattern three times and it should be fine.
Actually I would recommend changing the printing code above to something like:
```
sorted_results = sorted(test.result.microResults.items())
for key, micro_test in sorted_results:
```
to avoid the indeterministic output.


================
Comment at: utils/lit/tests/test-output-micro.py:9
+# CHECK-NEXT:   {
+# CHECK-NEXT:     "code": "PASS",
+# CHECK-NEXT:     "elapsed": null,
----------------
rengolin wrote:
> Here, the `CHECK-DAG` trick won't work, because you have two levels: one for the result and another for the metrics, and there's no way to identify `CHECK-DAG1` and `CHECK-DAG2`.
> 
> One way to solve this is to sort the keys before dumping JSON, as to get a reproducible answer (that is, of course, if Python's default sort is stable).
lit does the json dumping with `sort_keys=True` so the output should be deterministic so we don't need the `CHECK-DAG`s.


https://reviews.llvm.org/D43314