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

[James Henderson via Phabricator via libcxx-commits]

jhenderson added inline comments.


================
Comment at: llvm/utils/lit/lit/Test.py:216
+            with open(test_times_file, 'r') as time_file:
+                for line in time_file.readlines():
+                    time, path = line.split(' ', 1)
----------------
I believe you don't need the `readlines()` part of this line - my understanding is that you can iteratre over a file and it'll yield a line per iteration.


================
Comment at: llvm/utils/lit/lit/Test.py:217
+                for line in time_file.readlines():
+                    time, path = line.split(' ', 1)
+                    self.test_times[path.strip('\n')] = float(time)
----------------
You an probably omit the first argument here, as demonstrated. That will split on the first sequence of whitespace.


================
Comment at: llvm/utils/lit/lit/cl_arguments.py:154
     selection_group.add_argument("--shuffle",
-            help="Run tests in random order",
+            help="Start tests in random order",
             action="store_true")
----------------
I'd keep the old wording here and below (specifically "Start" -> "Run"). "Start" implies the tests might be started in a random order, but it may change mid run, which doesn't really make sense.


================
Comment at: llvm/utils/lit/lit/cl_arguments.py:214
+    if opts.incremental:
+        print('WARNING: --incremental was ignored. Failing tests now always start first.')
+
----------------
Maybe rather than "was ignored" use "is deprecated". Also "start" -> "run" as before.


================
Comment at: llvm/utils/lit/lit/main.py:175
+    else:
+        assert order == TestOrder.DEFAULT
+        tests.sort(key=lambda t: (not t.previous_failure, -t.previous_elapsed, t.getFullName()))
----------------
Perhaps worth a message with this assert to make it clear that the failure is due to an unknown TestOrder value.


================
Comment at: llvm/utils/lit/lit/main.py:273-274
+    for s, value in times_by_suite.items():
+        try:
+          path = os.path.join(s, '.lit_test_times.txt')
+          with open(path, 'w') as time_file:
----------------
Indentation here is inconsistent.


================
Comment at: llvm/utils/lit/tests/reorder.py:1
+## Check that we can reorder test starts
+
----------------
I don't really understand what you mean by "starts" here. Do you mean something like "Check that we can change the test order."

Also, missing full stop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179