[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

We agreed to forwards compatibility because people write big scripts that use the SB API, implement GUI's on top of them (more than just Xcode) etc.  So we try not to jerk those folks around.  That adds a little more responsibility on our part to think carefully about what we add, but the notion that we should refrain from making useful functionality available because we'd rather not be beholden to our decisions seems really wrong-headed to me.

And in this case there's a clear use for this. For instance the xnu macros have a bunch of Python based commands that spew out pages and pages of output.  Those guys would love to make their commands interruptible.  To do that they would need to call WasInterrupted.  So this is 100% something that should be available at the SB API layer.

Jim



> On Sep 19, 2017, at 11:18 AM, Zachary Turner <zturner at google.com> wrote:
> 
> Also, it avoids polluting the SB interface with another function that probably nobody is ever going to use outside of testing.  Adding to the SB API should take an act of God, given that once it gets added it has to stay until the end of time.
> 
> On Tue, Sep 19, 2017 at 11:15 AM Zachary Turner <zturner at google.com> wrote:
> That would work, but I think it's adding many more pieces to the test.  Now there's threads, a Python layer, and multiprocess dotest infrastructure in the equation.  Each providing an additional source of flakiness and instability.
> 
> If all it is is a pass-through, then just calling the function it passes through to is a strictly better test, since it's focusing the test on the underlying piece of functionality and removing additional sources of failure and instability from the test.
> 
> On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham <jingham at apple.com> wrote:
> I'd really prefer to do this as/along with an SB API test since we also need commands made through the SB API to be interruptible, and we should test that that also works.  So this kills two birds with one stone.
> 
> In general, when developing any high-level features in lldb one of the questions you should ask yourself is whether this is useful at the SB API layer.  In this case it obviously is (actually I'm a little embarrassed I didn't think of this requirement).  If so, then it's simplest to test it at that layer.  If the SB API is anything more than a pass-through, then you might also want to write a unit test for the underlying C++ API's.  But in this case the SB function will just call the underlying CommandInterpreter one, so testing at the SB layer is sufficient.
> 
> Jim
> 
> > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <reviews at reviews.llvm.org> wrote:
> >
> > zturner added a comment.
> >
> > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> >
> >> We should have a test. The test would need to use pexpect or something similar. If anyone has any pointer on how to make a test for this, please chime in. I was thinking just a pexpect based test.
> >
> >
> > This kind of thing is perfect for a unit test, but I'm not sure how easy that would be with the current design.  You'd basically do something like:
> >
> >  struct MockStream {
> >    explicit MockStream(CommandInterpreter &Interpreter, int InterruptAfter)
> >      : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter) {}
> >
> >    CommandInterpreter &Interpreter;
> >    const int InterruptAfter;
> >    int Lines = 0;
> >    std::string Buffer;
> >
> >    void Write(StringRef S) {
> >      ++Lines;
> >      if (Lines >= InterruptAfter) {
> >        Interpreter.Interrupt();
> >        return;
> >      }
> >      Buffer += S;
> >    }
> >  };
> >
> >  TEST_F(CommandInterruption) {
> >    CommandInterpreter Interpreter;
> >    MockStream Stream(Interpreter, 3);
> >    Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
> >    EXPECT_EQ(Stream.Lines == 3);
> >    EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
> >  }
> >
> >
> > https://reviews.llvm.org/D37923
> >
> >
> >
>