[Lldb-commits] [PATCH] D91193: [lldb] Fine tune expect() validation

[Dave Lee via Phabricator via lldb-commits]

kastiglione created this revision.
kastiglione added reviewers: jingham, teemperor, aprantl, JDevlieghere.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
kastiglione requested review of this revision.

Restore ability to call `expect()` with a message and no matcher.

After the changes in D88792 <https://reviews.llvm.org/D88792>, `expect` can no longer be called as:

  self.expect("some command", "some message")

After speaking to @jingham, I now understand that an uncommon use case is to
check only that a command succeeds, without validating its output. The changes
in D88792 <https://reviews.llvm.org/D88792> prevent such expectations.

This change restores the ability to call `expect` this way, but with the
requirement that the `msg` argument be a keyword argument. Requiring `msg` be a
keyword argument provides the ability to ensure `expect`'s arguments are
explict, and not ambiguous. Example:

  self.expect("some command", msg="some message")

When lldb supports only Python 3, the easy solution will be to change the
signature of `expect` by require all arguments be keyword ags using its `*`
syntax:

  def expect(
          self,
          str,
          *,
          msg=None,
          patterns=None,
          startstr=None,
          endstr=None,
          substrs=None,
          trace=False,
          error=False,
          ordered=True,
          matching=True,
          exe=True,
          inHistory=False):

But the `*` syntax is not supported in Python 2. To work around this, this change renames `expect` to `_expect_impl`, and implements `expect` as a separate function with a signature that separates positional args from keyword args, in order to perform validation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91193

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/test/API/assert_messages_test/TestAssertMessages.py


Index: lldb/test/API/assert_messages_test/TestAssertMessages.py
===================================================================
--- lldb/test/API/assert_messages_test/TestAssertMessages.py
+++ lldb/test/API/assert_messages_test/TestAssertMessages.py
@@ -122,10 +122,6 @@
         self.assert_expect_fails_with("any command",
             dict(substrs="some substring"),
             "substrs must be a collection of strings")
-        # Prevent `self.expect("cmd", "substr")`
-        self.assert_expect_fails_with("any command",
-            dict(msg="some substring"),
-            "expect() missing a matcher argument")
         # Prevent `self.expect("cmd", "msg", "substr")`
         self.assert_expect_fails_with("any command",
             dict(msg="a message", patterns="some substring"),
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===================================================================
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2388,7 +2388,35 @@
 
         self.assertTrue(cmd_status == 0)
 
-    def expect(
+
+    def expect(self, str, *args, **kwargs):
+        "Validate `expect` arguments before calling `_expect_impl`"
+        # Support success/fail only checks like:
+        #     self.expect("lldb command")
+        if not args and not kwargs:
+            return self._expect(str)
+
+        # Prevent this ambiguous mistake:
+        #     self.expect("lldb command", "some string")
+        # Instead, this invocation must be changed to either:
+        #     self.expect("lldb command", msg="some string")
+        # or use a matcher of some kind, such as:
+        #     self.expect("lldb command", substrs=["some string"])
+        if args:
+            needed = ("patterns", "startstr", "endstr", "substrs", "error")
+            assert any(arg in kwargs for arg in needed), \
+                "expect() requires a keyword argument"
+
+        # Check `patterns` and `substrs` are not accidentally given as strings.
+        assert not isinstance(kwargs.get("patterns"), six.string_types), \
+            "patterns must be a collection of strings"
+        assert not isinstance(kwargs.get("substrs"), six.string_types), \
+            "substrs must be a collection of strings"
+
+        return self._expect(str, **kwargs)
+
+
+    def _expect_impl(
             self,
             str,
             msg=None,
@@ -2429,22 +2457,6 @@
         set to False, the 'str' is treated as a string to be matched/not-matched
         against the golden input.
         """
-        # Catch cases where `expect` has been miscalled. Specifically, prevent
-        # this easy to make mistake:
-        #     self.expect("lldb command", "some substr")
-        # The `msg` parameter is used only when a failed match occurs. A failed
-        # match can only occur when one of `patterns`, `startstr`, `endstr`, or
-        # `substrs` has been given. Thus, if a `msg` is given, it's an error to
-        # not also provide one of the matcher parameters.
-        if msg and not (patterns or startstr or endstr or substrs or error):
-            assert False, "expect() missing a matcher argument"
-
-        # Check `patterns` and `substrs` are not accidentally given as strings.
-        assert not isinstance(patterns, six.string_types), \
-            "patterns must be a collection of strings"
-        assert not isinstance(substrs, six.string_types), \
-            "substrs must be a collection of strings"
-
         trace = (True if traceAlways else trace)
 
         if exe:


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D91193.304274.patch
Type: text/x-patch
Size: 3596 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20201110/398b6d66/attachment.bin>