[Lldb-commits] [lldb] [lldb][test] Add `pexpect` category for tests that `import pexpect` (PR #84860)

[Jordan Rupprecht via lldb-commits]

================
@@ -914,6 +914,25 @@ def checkForkVForkSupport():
         configuration.skip_categories.append("fork")
 
 
+def checkPexpectSupport():
+    from lldbsuite.test import lldbplatformutil
+
+    platform = lldbplatformutil.getPlatform()
+
+    # llvm.org/pr22274: need a pexpect replacement for windows
+    if platform in ["windows"]:
+        if configuration.verbose:
+            print("pexpect tests will be skipped because of unsupported platform")
+        configuration.skip_categories.append("pexpect")
+    elif not configuration.shouldSkipBecauseOfCategories(["pexpect"]):
+        try:
+            import pexpect
+        except:
+            print(
+                "Warning: pexpect is not installed, but pexpect tests are not being skipped."
----------------
rupprecht wrote:

Proposed some more specific wording in https://github.com/llvm/llvm-project/pull/84860/commits/bcfc5e8f2f18e8eb8f5a96e18c649f3cb2e248c0

But this `elif` block is the least thing that I care about actually landing :) I think it's helpful, but if this is contentious, it's not a big deal to drop it and just have a potentially worse error message. So I dropped it altogether in https://github.com/llvm/llvm-project/pull/84860/commits/03e13152eac8416c31414b4f469e281c40b80deb. I can always add it back if needed, either in this PR or as a followup commit.

> So what would happens without this code? Wouldn't we try to import pexpect anyway in the relevant test? In other words, do we even need this?

Yep, see the snippet above ([link](#discussion_r1521684820)) for what it'd look like if we just remove it. Having a specific check+error text here is not essential, but might help some developer if they aren't aware of how to skip this category.

> It seems that printing a warning for non-pexpect either adds noise to the already verbose test output or will be invisible if the test passes.

Agreed about spamminess; that was changed in https://github.com/llvm/llvm-project/pull/84860/commits/7f2ec70a35d1f5426afcf354f9b16b0052e81df2 to make this a hard error.

> If the error message is poor enough that we would want to print something more actionable, could we do it from the PExpectTest base class?

That would not work for tests that use pexpect outside of `PExpectTest`, which are:
- lldb/test/API/terminal/TestSTTYBeforeAndAfter.py
- lldb/test/API/macosx/nslog/TestDarwinNSLogOutput.py
- lldb/test/API/benchmarks/expression/TestRepeatedExprs.py (and many other benchmark tests)

So if the user just runs `ninja check-lldb`, they'd get an actionable message from PExpectTest-based tests, but a potentially confusing one from TestSTTYBeforeAndAfter.

https://github.com/llvm/llvm-project/pull/84860