[libcxx-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

[Julian Lettner via Phabricator via libcxx-commits]

yln added a comment.

Very happy with the patch!  Thanks for all your hard work! :)
I have a few small nits and one ask: can we model "test failed" explicitly instead of making their execution time really large?



================
Comment at: llvm/utils/lit/lit/discovery.py:141
+    test_times = {}
+    test_times_file = ts.exec_root + os.sep + '.lit_test_times.txt'
+    if not os.path.exists(test_times_file):
----------------
Use `os.path.join(...)`


================
Comment at: llvm/utils/lit/lit/discovery.py:145-149
+        file = open(test_times_file, 'r')
+        for line in file.readlines():
+            time, path = line.split(' ', 1)
+            test_times[path.strip('\n')] = float(time)
+        file.close()
----------------
Use Python's `with` statement


================
Comment at: llvm/utils/lit/lit/main.py:108
 
+    record_test_times(discovered_tests, lit_config)
+
----------------
I think it would be sufficient here to pass in the tests that we attempted to run.


================
Comment at: llvm/utils/lit/lit/main.py:166
 
 def determine_order(tests, order):
     from lit.cl_arguments import TestOrder
----------------
Really like all the simplifications this enabled! :)


================
Comment at: llvm/utils/lit/lit/main.py:213
 
     run = lit.run.Run(tests, lit_config, workers, progress_callback,
                       opts.max_failures, opts.timeout)
----------------
Is it possible to directly pass the display callback here now (and remove the local `progress_callback()`)?


================
Comment at: llvm/utils/lit/lit/main.py:270-271
+        time = t.result.elapsed
+        if t.isFailure():
+            time += 1.0e10
+        times_by_suite[t.suite.exec_root].append((os.sep.join(t.path_in_suite), t.result.elapsed))
----------------
Can we explicitly model this part in code instead of increasing the execution time of failed tests?  We can use the `test.result.code` property.


================
Comment at: llvm/utils/lit/lit/main.py:274-275
+
+    if not times_by_suite:
+        return
+
----------------
Not really needed and can be removed, right?


================
Comment at: llvm/utils/lit/lit/main.py:277-286
+    for s, value in times_by_suite.items():
+        try:
+          path = s + os.sep + '.lit_test_times.txt'
+          file = open(path, 'w')
+          for name, time in value:
+              file.write(("%e" % time) + ' ' + name + '\n')
+          file.close()
----------------
Using Python's `with` and `os.path` and string interpolation


================
Comment at: llvm/utils/lit/tests/Inputs/reorder/.lit_test_times.txt:3
+2.0 bbb.txt
+0.1 aaa.txt
----------------
Clarifying my above comment:
```
PASS 3.0 subdir/ccc.txt
FAIL 2.0 bbb.txt
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179