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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
