[unittest] Add option to allow disabling sharding in unittest

[zeroomega]

[unittest] Add lit option to allow disabling sharding in unittest

By default, googletest based unit tests uses sharding to speed up the testing. However, when these unit tests are through wrapper program on a remote platform with large round trip time, the sharding will increase
the time cost dramatically. This patch adds a LLVM LIT option "--disable-gtest-sharding" to allow sharding on gtest based unittest to be disabled.

[llvmbot]

@llvm/pr-subscribers-testing-tools

Changes

[unittest] Add option to allow disabling sharding in unittest

By default, googletest based unit tests uses sharding to speed up the
testing. However, when these unit tests are through wrapper program on a
remote platform with large round trip time, the sharding will increase
the time cost dramatically. This patch adds an "use_sharding" in the lit
googletest configuration and it can be configured using
"LLVM_GTEST_USE_SHARDING" cmake option.

---

Full diff: https://github.com/llvm/llvm-project/pull/67063.diff

6 Files Affected:

• (modified) llvm/test/Unit/lit.cfg.py (+6-1)
• (modified) llvm/test/Unit/lit.site.cfg.py.in (+2)
• (modified) llvm/utils/lit/lit/formats/googletest.py (+72-27)
• (added) llvm/utils/lit/tests/Inputs/googletest-no-sharding/DummySubDir/OneTest.py (+105)
• (added) llvm/utils/lit/tests/Inputs/googletest-no-sharding/lit.cfg (+4)
• (added) llvm/utils/lit/tests/googletest-no-sharding.py (+43)

diff --git a/llvm/test/Unit/lit.cfg.py b/llvm/test/Unit/lit.cfg.py
index f15c30dbcdb0aa6..4e815444d339dcd 100644
--- a/llvm/test/Unit/lit.cfg.py
+++ b/llvm/test/Unit/lit.cfg.py
@@ -19,7 +19,12 @@
 config.test_source_root = config.test_exec_root
 
 # testFormat: The test format to use to interpret tests.
-config.test_format = lit.formats.GoogleTest(config.llvm_build_mode, "Tests")
+config.test_format = lit.formats.GoogleTest(
+    config.llvm_build_mode,
+    "Tests",
+    run_under=config.gtest_run_under,
+    use_sharding=config.use_gtest_sharding,
+)
 
 # Propagate the temp directory. Windows requires this because it uses \Windows\
 # if none of these are present.
diff --git a/llvm/test/Unit/lit.site.cfg.py.in b/llvm/test/Unit/lit.site.cfg.py.in
index 1d7d76580149495..523d2bb3817e122 100644
--- a/llvm/test/Unit/lit.site.cfg.py.in
+++ b/llvm/test/Unit/lit.site.cfg.py.in
@@ -7,6 +7,8 @@ config.llvm_obj_root = path(r"@LLVM_BINARY_DIR@")
 config.llvm_tools_dir = lit_config.substitute(path(r"@LLVM_TOOLS_DIR@"))
 config.llvm_build_mode = lit_config.substitute("@LLVM_BUILD_MODE@")
 config.shlibdir = lit_config.substitute(path(r"@SHLIBDIR@"))
+config.gtest_run_under = lit_config.substitute(r"@LLVM_GTEST_RUN_UNDER@")
+config.use_gtest_sharding = lit_config.substitute(r"@LLVM_GTEST_USE_SHARD@")
 
 # Let the main config do the real work.
 lit_config.load_config(
diff --git a/llvm/utils/lit/lit/formats/googletest.py b/llvm/utils/lit/lit/formats/googletest.py
index f8304cbd05453b5..f05e7e351cfefa6 100644
--- a/llvm/utils/lit/lit/formats/googletest.py
+++ b/llvm/utils/lit/lit/formats/googletest.py
@@ -15,7 +15,7 @@
 
 
 class GoogleTest(TestFormat):
-    def __init__(self, test_sub_dirs, test_suffix, run_under=[]):
+    def __init__(self, test_sub_dirs, test_suffix, run_under=[], use_sharding=True):
         self.seen_executables = set()
         self.test_sub_dirs = str(test_sub_dirs).split(";")
 
@@ -27,6 +27,17 @@ def __init__(self, test_sub_dirs, test_suffix, run_under=[]):
         # Also check for .py files for testing purposes.
         self.test_suffixes = {exe_suffix, test_suffix + ".py"}
         self.run_under = run_under
+        if type(use_sharding) is str:
+            use_sharding = use_sharding.lower()
+            if use_sharding in ("0", "off", "false", "no", "n", "ignore", "notfound"):
+                use_sharding = False
+            else:
+                use_sharding = True
+        elif type(use_sharding) is bool:
+            pass
+        else:
+            use_sharding = True
+        self.use_sharding = use_sharding
 
     def get_num_tests(self, path, litConfig, localConfig):
         list_test_cmd = self.prepareCmd(
@@ -68,24 +79,49 @@ def getTestsInDirectory(self, testSuite, path_in_suite, litConfig, localConfig):
                     self.seen_executables.add(execpath)
                 num_tests = self.get_num_tests(execpath, litConfig, localConfig)
                 if num_tests is not None:
-                    # Compute the number of shards.
-                    shard_size = init_shard_size
-                    nshard = int(math.ceil(num_tests / shard_size))
-                    while nshard < core_count and shard_size > 1:
-                        shard_size = shard_size // 2
+                    if self.use_sharding:
+                        # Compute the number of shards.
+                        shard_size = init_shard_size
                         nshard = int(math.ceil(num_tests / shard_size))
-
-                    # Create one lit test for each shard.
-                    for idx in range(nshard):
-                        testPath = path_in_suite + (subdir, fn, str(idx), str(nshard))
+                        while nshard < core_count and shard_size > 1:
+                            shard_size = shard_size // 2
+                            nshard = int(math.ceil(num_tests / shard_size))
+
+                        # Create one lit test for each shard.
+                        for idx in range(nshard):
+                            testPath = path_in_suite + (
+                                subdir,
+                                fn,
+                                str(idx),
+                                str(nshard),
+                            )
+                            json_file = (
+                                "-".join(
+                                    [
+                                        execpath,
+                                        testSuite.config.name,
+                                        str(os.getpid()),
+                                        str(idx),
+                                        str(nshard),
+                                    ]
+                                )
+                                + ".json"
+                            )
+                            yield lit.Test.Test(
+                                testSuite,
+                                testPath,
+                                localConfig,
+                                file_path=execpath,
+                                gtest_json_file=json_file,
+                            )
+                    else:
+                        testPath = path_in_suite + (subdir, fn)
                         json_file = (
                             "-".join(
                                 [
                                     execpath,
                                     testSuite.config.name,
                                     str(os.getpid()),
-                                    str(idx),
-                                    str(nshard),
                                 ]
                             )
                             + ".json"
@@ -118,24 +154,33 @@ def execute(self, test, litConfig):
         if test.gtest_json_file is None:
             return lit.Test.FAIL, ""
 
-        testPath, testName = os.path.split(test.getSourcePath())
-        while not os.path.exists(testPath):
-            # Handle GTest parametrized and typed tests, whose name includes
-            # some '/'s.
-            testPath, namePrefix = os.path.split(testPath)
-            testName = namePrefix + "/" + testName
-
-        testName, total_shards = os.path.split(testName)
-        testName, shard_idx = os.path.split(testName)
+        testPath = test.getSourcePath()
         from lit.cl_arguments import TestOrder
 
         use_shuffle = TestOrder(litConfig.order) == TestOrder.RANDOM
-        shard_env = {
-            "GTEST_OUTPUT": "json:" + test.gtest_json_file,
-            "GTEST_SHUFFLE": "1" if use_shuffle else "0",
-            "GTEST_TOTAL_SHARDS": os.environ.get("GTEST_TOTAL_SHARDS", total_shards),
-            "GTEST_SHARD_INDEX": os.environ.get("GTEST_SHARD_INDEX", shard_idx),
-        }
+        if self.use_sharding:
+            testPath, testName = os.path.split(test.getSourcePath())
+            while not os.path.exists(testPath):
+                # Handle GTest parametrized and typed tests, whose name includes
+                # some '/'s.
+                testPath, namePrefix = os.path.split(testPath)
+                testName = namePrefix + "/" + testName
+
+            testName, total_shards = os.path.split(testName)
+            testName, shard_idx = os.path.split(testName)
+            shard_env = {
+                "GTEST_OUTPUT": "json:" + test.gtest_json_file,
+                "GTEST_SHUFFLE": "1" if use_shuffle else "0",
+                "GTEST_TOTAL_SHARDS": os.environ.get(
+                    "GTEST_TOTAL_SHARDS", total_shards
+                ),
+                "GTEST_SHARD_INDEX": os.environ.get("GTEST_SHARD_INDEX", shard_idx),
+            }
+        else:
+            shard_env = {
+                "GTEST_OUTPUT": "json:" + test.gtest_json_file,
+                "GTEST_SHUFFLE": "1" if use_shuffle else "0",
+            }
         test.config.environment.update(shard_env)
 
         cmd = [testPath]
diff --git a/llvm/utils/lit/tests/Inputs/googletest-no-sharding/DummySubDir/OneTest.py b/llvm/utils/lit/tests/Inputs/googletest-no-sharding/DummySubDir/OneTest.py
new file mode 100644
index 000000000000000..8fcab0081a21209
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/googletest-no-sharding/DummySubDir/OneTest.py
@@ -0,0 +1,105 @@
+#!/usr/bin/env python
+
+import os
+import sys
+
+if len(sys.argv) == 3 and sys.argv[1] == "--gtest_list_tests":
+    if sys.argv[2] != "--gtest_filter=-*DISABLED_*":
+        raise ValueError("unexpected argument: %s" % (sys.argv[2]))
+    print(
+        """\
+FirstTest.
+  subTestA
+  subTestB
+  subTestC
+  subTestD
+ParameterizedTest/0.
+  subTest
+ParameterizedTest/1.
+  subTest"""
+    )
+    sys.exit(0)
+elif len(sys.argv) != 1:
+    # sharding and json output are specified using environment variables
+    raise ValueError("unexpected argument: %r" % (" ".join(sys.argv[1:])))
+
+for e in ["GTEST_OUTPUT"]:
+    if e not in os.environ:
+        raise ValueError("missing environment variables: " + e)
+
+if not os.environ["GTEST_OUTPUT"].startswith("json:"):
+    raise ValueError("must emit json output: " + os.environ["GTEST_OUTPUT"])
+
+output = """\
+{
+"random_seed": 123,
+"testsuites": [
+    {
+        "name": "FirstTest",
+        "testsuite": [
+            {
+                "name": "subTestA",
+                "result": "COMPLETED",
+                "time": "0.001s"
+            },
+            {
+                "name": "subTestB",
+                "result": "COMPLETED",
+                "time": "0.001s",
+                "failures": [
+                    {
+                        "failure": "I am subTest B, I FAIL\\nAnd I have two lines of output",
+                        "type": ""
+                    }
+                ]
+            },
+            {
+                "name": "subTestC",
+                "result": "SKIPPED",
+                "time": "0.001s"
+            },
+            {
+                "name": "subTestD",
+                "result": "UNRESOLVED",
+                "time": "0.001s"
+            }
+        ]
+    },
+    {
+        "name": "ParameterizedTest/0",
+        "testsuite": [
+            {
+                "name": "subTest",
+                "result": "COMPLETED",
+                "time": "0.001s"
+            }
+        ]
+    },
+    {
+        "name": "ParameterizedTest/1",
+        "testsuite": [
+            {
+                "name": "subTest",
+                "result": "COMPLETED",
+                "time": "0.001s"
+            }
+        ]
+    }
+]
+}"""
+
+dummy_output = """\
+{
+"testsuites": [
+]
+}"""
+
+json_filename = os.environ["GTEST_OUTPUT"].split(":", 1)[1]
+with open(json_filename, "w", encoding="utf-8") as f:
+    print("[ RUN      ] FirstTest.subTestB", flush=True)
+    print("I am subTest B output", file=sys.stderr, flush=True)
+    print("[  FAILED  ] FirstTest.subTestB (8 ms)", flush=True)
+    f.write(output)
+    exit_code = 1
+
+sys.exit(exit_code)
diff --git a/llvm/utils/lit/tests/Inputs/googletest-no-sharding/lit.cfg b/llvm/utils/lit/tests/Inputs/googletest-no-sharding/lit.cfg
new file mode 100644
index 000000000000000..5507fe65782dcbc
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/googletest-no-sharding/lit.cfg
@@ -0,0 +1,4 @@
+import lit.formats
+
+config.name = "googletest-no-sharding"
+config.test_format = lit.formats.GoogleTest("DummySubDir", "Test", use_sharding="False")
diff --git a/llvm/utils/lit/tests/googletest-no-sharding.py b/llvm/utils/lit/tests/googletest-no-sharding.py
new file mode 100644
index 000000000000000..3fa7a37cdd7090c
--- /dev/null
+++ b/llvm/utils/lit/tests/googletest-no-sharding.py
@@ -0,0 +1,43 @@
+# Check the various features of the GoogleTest format.
+
+# RUN: not %{lit} -v --order=random %{inputs}/googletest-no-sharding > %t.out
+# FIXME: Temporarily dump test output so we can debug failing tests on
+# buildbots.
+# RUN: cat %t.out
+# RUN: FileCheck < %t.out %s
+#
+# END.
+
+# CHECK: -- Testing:
+# CHECK: FAIL: googletest-no-sharding :: [[PATH:[Dd]ummy[Ss]ub[Dd]ir/]][[FILE:OneTest\.py]]
+# CHECK: *** TEST 'googletest-no-sharding :: [[PATH]][[FILE]]' FAILED ***
+# CHECK-NEXT: Script(shard):
+# CHECK-NEXT: --
+# CHECK-NEXT: GTEST_OUTPUT=json:{{[^[:space:]]*}} GTEST_SHUFFLE=1 GTEST_RANDOM_SEED=123 {{.*}}[[FILE]]
+# CHECK-NEXT: --
+# CHECK-EMPTY:
+# CHECK-NEXT: Script:
+# CHECK-NEXT: --
+# CHECK-NEXT: [[FILE]] --gtest_filter=FirstTest.subTestB
+# CHECK-NEXT: --
+# CHECK-NEXT: I am subTest B output
+# CHECK-EMPTY:
+# CHECK-NEXT: I am subTest B, I FAIL
+# CHECK-NEXT: And I have two lines of output
+# CHECK-EMPTY:
+# CHECK: Script:
+# CHECK-NEXT: --
+# CHECK-NEXT: [[FILE]] --gtest_filter=FirstTest.subTestD
+# CHECK-NEXT: --
+# CHECK-NEXT: unresolved test result
+# CHECK: ***
+# CHECK: ***
+# CHECK: Unresolved Tests (1):
+# CHECK-NEXT:   googletest-no-sharding :: FirstTest/subTestD
+# CHECK: ***
+# CHECK-NEXT: Failed Tests (1):
+# CHECK-NEXT:   googletest-no-sharding :: FirstTest/subTestB
+# CHECK: Skipped{{ *}}: 1
+# CHECK: Passed{{ *}}: 3
+# CHECK: Unresolved{{ *}}: 1
+# CHECK: Failed{{ *}}: 1

[arichardson]

I see there are lots more calls to lit.formats.GoogleTest() across the tree, shouldn't those also be updated?

[zeroomega]

The motivation behind the 2 patches is we would like to extend LLVM support for a Fuchsia platform and be able to use LLVM library natively on Fuchsia. As a first step, we need to make sure LLVM test (starting from the LLVM unittest) can run successfully on Fuchsia. And it can only be achieved by cross compile LLVM on a supported host like Linux but run the test remotely on Fuchsia through a wrapper that bring a tunnel between the host and testing target platform. Sharding is causing significant time overhead in this use case, so as the first step, I add the option to allow LLVM unitest sharding to be disabled. The option's name is also -DLLVM_GTEST_USE_SHARDING so it only apply to LLVM's gtest based unit test. If other individual component like compiler-rt needs the same feature, IMO should be using -DRUNTIME_GTEST_USE_SHARDING

But I get your point, if you think a global setting to disable sharding on gtest is needed, maybe we should consider a LIT's configuration and make it configurable through -DLLVM_LIT_ARGS= so we don't have to individually modify each test configuration. Please let me know your comment on this.

[petrhosek]

I think that making this behavior configurable through a lit argument which could be set through -DLLVM_LIT_ARGS= would be cleaner.

[zeroomega]

I have re-implemented as an LIT option and it can be configured using -DLLVM_LIT_ARGS=--disable-gtest-sharding. Please take a look.

[JoeLoser]

Thanks for tackling this - it may be useful for those running into #56492 or derivative issues. I ran into that issue in the past a year ago.

[MaskRay]

(The previous title "[unittest] Add lit option to allow disabling sharding in unittest") was in the description, so commit 6eee238 got a duplicated title.)

[zeroomega]

My bad. I didn't noticed that when I hit the squash and land button. Will pay attention next time.

[petrhosek]

This variable name is inconsistent with Python style which uses snake_case. I'd suggest calling it simply gtest_sharding and have a pair of arguments --gtest-sharding and --no-gtest-sharding to control whether sharding is enabled or disabled, with default being True.

[petrhosek]

Looks like my comment wasn't addressed, it'd be great to address this in a follow up change.