[lit] Harmonize test timing data between Unix and Windows

[davezarzycki]

Summary

The "path" recorded for timing purposes is only used as a key into a dictionary. It is never used as an actual path to a filesystem API, therefore we should use '/' as the canonical separator so that Unix and Windows machines can share timing data. This also ensures that the lit testing works across platforms.

[jhenderson]

@jmorse, could you try this out to see if it fixes the issues you were seeing before? LGTM, assuming it fixes the Windows issue.

[jmorse]

Passes on Windows just fine, thanks for the follow up!

[yln]

llvm/utils/lit/lit/Test.py

265–266

Could we use getFullName() here? Or if the config-qualified name is undesirable, we could extract this into Test::getName().

def getFullName(self):
    return self.suite.config.name + ' :: ' + '/'.join(self.path_in_suite)

[davezarzycki]

llvm/utils/lit/lit/Test.py

265–266

All of the timing data is stored per test suite, so prefixing the suite name in this context doesn't solve anything. As to factoring that out, sure. We can do that.

llvm/utils/lit/lit/Test.py

def __init__(self, suite, path_in_suite, config, file_path = None):
         self.result = None
 
         # The previous test failure state, if applicable.
         self.previous_failure = False
 
         # The previous test elapsed time, if applicable.
         self.previous_elapsed = 0.0
 
-        if os.sep.join(path_in_suite) in suite.test_times:
-            time = suite.test_times[os.sep.join(path_in_suite)]
+        if '/'.join(path_in_suite) in suite.test_times:
+            time = suite.test_times['/'.join(path_in_suite)]
             self.previous_elapsed = abs(time)
             self.previous_failure = time < 0
 
 
     def setResult(self, result):
         assert self.result is None, "result already set"
         assert isinstance(result, Result), "unexpected result type"
         try:

llvm/utils/lit/lit/main.py

 def record_test_times(tests, lit_config):
     times_by_suite = {}
     for t in tests:
         if not t.result.elapsed:
             continue
         if not t.suite.exec_root in times_by_suite:
             times_by_suite[t.suite.exec_root] = []
         time = -t.result.elapsed if t.isFailure() else t.result.elapsed
-        times_by_suite[t.suite.exec_root].append((os.sep.join(t.path_in_suite), t.result.elapsed))
+        # The "path" here is only used as a key into a dictionary. It is never
+        # used as an actual path to a filesystem API, therefore we use '/' as
+        # the canonical separator so that Unix and Windows machines can share
+        # timing data.
+        times_by_suite[t.suite.exec_root].append(('/'.join(t.path_in_suite),
+          t.result.elapsed))
 
     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:
                 for name, time in value:
                     time_file.write(("%e" % time) + ' ' + name + '\n')
         except:

llvm/utils/lit/tests/reorder.py

 ## Check that we can reorder test runs.
 
 # RUN: cp %{inputs}/reorder/.lit_test_times.txt %{inputs}/reorder/.lit_test_times.txt.orig
 # RUN: %{lit} -j1 %{inputs}/reorder | FileCheck %s
 # RUN: not diff %{inputs}/reorder/.lit_test_times.txt %{inputs}/reorder/.lit_test_times.txt.orig
-# UNSUPPORTED: system-windows
 # END.
 
 # CHECK:     -- Testing: 3 tests, 1 workers --
 # CHECK-NEXT: PASS: reorder :: subdir/ccc.txt
 # CHECK-NEXT: PASS: reorder :: bbb.txt
 # CHECK-NEXT: PASS: reorder :: aaa.txt
 # CHECK:     Passed: 3