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

[Julian Lettner via Phabricator via lldb-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

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Julian Lettner via Phabricator via lldb-commits]

yln added a comment.

In D98179#2629578 , @davezarzycki 
wrote:

> ... I think we should use slash as the canonical separator for the timing 
> data file.

+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits