[PATCH] D122251: [lit] Use sharding for GoogleTest format

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In D122251#3414840 <https://reviews.llvm.org/D122251#3414840>, @ychen wrote:

> It would trigger an assertion failure :  `assert os.path.exists(test.gtest_json_file)`. The only way this could happen is the copy of GoogleTest in LLVM has crashing bugs that may be triggered by new tests or new test environments (existing ones, if any, should be found when this patch is landed). I think both cases could be detected and handled reliably.

I think I don't agree that the only way this could happen is if there are gtest bugs. One segfault in the middle of a test suite (on Linux, no SEH or signal handler recovery mechanism) will bring down the entire gtest shard process.



================
Comment at: llvm/utils/lit/lit/formats/googletest.py:113
 
-        if exitCode:
-            return lit.Test.FAIL, header + out + err
-
-        if '[  SKIPPED ] 1 test,' in out:
-            return lit.Test.SKIPPED, ''
-
-        passing_test_line = '[  PASSED  ] 1 test.'
-        if passing_test_line not in out:
-            return (lit.Test.UNRESOLVED,
-                    f'{header}Unable to find {passing_test_line} '
-                    f'in gtest output:\n\n{out}{err}')
+        assert os.path.exists(test.gtest_json_file)
 
----------------
I don't think it makes sense to use `assert` to check for things that are not expected invariants. If an exception is desired, IMO it's better to use `if ...: raise ...`. Alternatively, it makes sense to return a FAIL test status in this case.


================
Comment at: llvm/utils/lit/lit/formats/googletest.py:124
+                        testname = testcase['name'] + '.' + testinfo['name']
+                        header = f"Script:\n--\n{' '.join(cmd)} --gtest_filter={testname}\n--\n"
+                        if 'failures' in testinfo:
----------------
This command is different from the sharded command. It is possible to construct tests that fail when run together but pass when run in isolation, so this command may not always reproduce failures observable with lit. I'm not sure how to signal that to the user.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122251