[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Raphael Isemann via Phabricator via lldb-commits]

teemperor marked an inline comment as done.
teemperor added inline comments.


================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2480-2482
+        self.expect_expr(var, result_value=result_value, result_type=result_type, run_type=self.ExpressionCommandType.EXPRESSION)
+        self.expect_expr(var, result_value=result_value, result_type=result_type, run_type=self.ExpressionCommandType.FRAME_VAR)
+        self.expect_expr(var, result_value=result_value, result_type=result_type, run_type=self.ExpressionCommandType.EXPRESSION)
----------------
labath wrote:
> teemperor wrote:
> > labath wrote:
> > > I'm also not sold on the idea of running the same expression multiple times just because we've had some bug that would've been caught by that in the past. Lldb already does a lot more combinatorial testing than anything else in llvm. I don't think that adding more of it is the solution to any stability problem.
> > > 
> > > If there's some tricky aspect of combining "frame variable" and "expression" commands then we should have a separate test for that. I'd be much happier to have one or two tests that run a single expression a hundred times than putting the repetition in every test and hoping the shotgun effect of that will catch something.
> > I'm fine making this just `frame var`/`expr` and not `expr`/`frame`/`expr`, but I don't see how we can get rid of the fact that we need to test both APIs (which is not just about them interacting, but just that we need to test both unique code paths). Testing just one is not a responsible way of testing these APIs and duplicating calls for them in all relevant tests doesn't sound like an option either.
> Both apis need to be tested, of course, and having some utility to do that seems reasonable. A different question is how when should this utility be used.
> 
> For instance, one could reasonable argue that "expression" is not needed for testing data formatters, as they only kick in after the result has been computed (and they should not care about how that result came to be). While OTOH, formatters have some pretty complex interactions with the "frame variable" command. However, it seems that both mechanisms are being used right now for testing formatters, so I am not going to argue for removing them.
> 
> I also don't think it's a disaster if someone runs both apis because "it's easy to do so", though I still think it's better (for various reasons) to write more isolated tests whereever possible.
>From a reasonable point of view, data formatters should only be tested with one of them. However, our implementation isn't reasonable as the data formatters for `frame var` and `expr` work with completely differently computed ASTs under the hood (with `expr` we have this fragile indirection via the temporary AST for the expression). Testing the data formatters with both `expr` and `frame var` is one of the main motivations for doing this.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D70314