[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

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

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/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 += "Run with environment: %s\n" % env
+                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)