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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
