[libcxx] Improve reporting when running the lit test suite

[ldionne]

Summary

Running the test suite with -a will now properly show all the executed
commands. The reports also include the environment under which the test
is being executed, which is helpful for reproducing issues.

[EricWF]

Could you elaborate as to why this is needed/beneficial? Most of the information this patch adds to reports is already displayed to the user during the initial configuration.

libcxx/utils/libcxx/test/format.py

210 ↗

(On Diff #169485)

The relevant environment variables should be printed once at the start of the run. IDK why we need to display them again here. I think it only adds noise.

The same goes for the compile command. We should dump all the relevant compile flags at the start of the run during initial configuration.

214 ↗

(On Diff #169485)

How is printing the name of the temporary executable file here helpful?

[EricWF]

Setting "Requested Changes" to move this from my TODO review queue.

[ldionne]

In D53215#1264337, @EricWF wrote:

Could you elaborate as to why this is needed/beneficial? Most of the information this patch adds to reports is already displayed to the user during the initial configuration.

This is useful whenever you try to see why a test is passing when you think it should fail. In my case, I was trying to figure out what was different between two lit invocations: a test was failing on a CI but it was passing on my local machine (i.e. I couldn't reproduce the failure). I tried adding -a thinking "this is going to tell me all I need to know", but it didn't. So instead I had to make the test fail artificially, look at what the command-line was (because that is printed when the test fails), and try to reproduce that way. Much better would be to just run the test under -a, which is advertised by lit as dumping everything.

There is some amount of crap in what's printed, e.g. temporary executable files, etc. These can be edited manually when working on a reproduction. Also note that you should not normally use -a when running lit (and I don't think people usually do that). You use -a when you really want to see what's going on, and this patch gives you that information.

[EricWF]

LGTM without the environment. If we need more information about the ENV we can print it once, during the initial configuration of the test suite.

[ldionne]

Committed without the environment.

libcxx/trunk/utils/libcxx/test/format.py

def _evaluate_pass_test(self, test, tmpBase, lit_config,
             # Compile the test
             cmd, out, err, rc = test_cxx.compileLinkTwoSteps(
                 source_path, out=exec_path, object_file=object_path,
                 cwd=execDir)
             compile_cmd = cmd
             if rc != 0:
                 report = libcxx.util.makeReport(cmd, out, err, rc)
                 report += "Compilation failed unexpectedly!"
-                return lit.Test.FAIL, report
+                return lit.Test.Result(lit.Test.FAIL, report)
             # Run the test
             local_cwd = os.path.dirname(source_path)
             env = None
             if self.exec_env:
                 env = self.exec_env
             # TODO: Only list actually needed files in file_deps.
             # Right now we just mark all of the .dat files in the same
             # directory as dependencies, but it's likely less than that. We
             # should add a `// FILE-DEP: foo.dat` to each test to track this.
             data_files = [os.path.join(local_cwd, f)
                           for f in os.listdir(local_cwd) if f.endswith('.dat')]
             is_flaky = self._get_parser('FLAKY_TEST.', parsers).getValue()
             max_retry = 3 if is_flaky else 1
             for retry_count in range(max_retry):
                 cmd, out, err, rc = self.executor.run(exec_path, [exec_path],
                                                       local_cwd, data_files,
                                                       env)
+                report = "Compiled With: %s\n" % compile_cmd
+                report += libcxx.util.makeReport(cmd, out, err, rc)
                 if rc == 0:
                     res = lit.Test.PASS if retry_count == 0 else lit.Test.FLAKYPASS
-                    return res, ''
+                    return lit.Test.Result(res, report)
                 elif rc != 0 and retry_count + 1 == max_retry:
-                    report = libcxx.util.makeReport(cmd, out, err, rc)
-                    report = "Compiled With: %s\n%s" % (compile_cmd, report)
                     report += "Compiled test failed unexpectedly!"
-                    return lit.Test.FAIL, report
+                    return lit.Test.Result(lit.Test.FAIL, report)
 
             assert False # Unreachable
         finally:
             # Note that cleanup of exec_file happens in `_clean()`. If you
             # override this, cleanup is your reponsibility.
             libcxx.util.cleanFile(object_path)
             self._clean(exec_path)
 
def _evaluate_fail_test(self, test, test_cxx, parsers):
             #
             # Therefore, we check if the test was expected to fail because of
             # nodiscard before enabling it
             test_str_list = ['ignoring return value', 'nodiscard', 'NODISCARD']
             if any(test_str in contents for test_str in test_str_list):
                 test_cxx.flags += ['-Werror=unused-result']
         cmd, out, err, rc = test_cxx.compile(source_path, out=os.devnull)
         expected_rc = 0 if use_verify else 1
+        report = libcxx.util.makeReport(cmd, out, err, rc)
         if rc == expected_rc:
-            return lit.Test.PASS, ''
+            return lit.Test.Result(lit.Test.PASS, report)
         else:
-            report = libcxx.util.makeReport(cmd, out, err, rc)
-            report_msg = ('Expected compilation to fail!' if not use_verify else
-                          'Expected compilation using verify to pass!')
-            return lit.Test.FAIL, report + report_msg + '\n'
+            report += ('Expected compilation to fail!\n' if not use_verify else
+                       'Expected compilation using verify to pass!\n')
+            return lit.Test.Result(lit.Test.FAIL, report)