teemperor planned changes to this revision. teemperor marked an inline comment as done. teemperor added a comment.
In D70314#1749427 <https://reviews.llvm.org/D70314#1749427>, @labath wrote: > In D70314#1747850 <https://reviews.llvm.org/D70314#1747850>, @teemperor wrote: > > > In D70314#1747711 <https://reviews.llvm.org/D70314#1747711>, @labath wrote: > > > > > Semi-random idea: Instead of running the expression through the command > > > line and then unpacking the result, should we just use the appropriate > > > SBFrame api here (EvaluateExpression/GetValueForVariablePath)? > > > > > > That's one of the APIs we need to test, but we need to test all of them to > > prevent regressions like D67803 <https://reviews.llvm.org/D67803>. At least > > `frame var` and `expr` behave very differently (EvaluateExpression should > > be similar to `expr` but I never looked at that code path). So the idea is > > to have one command that tests all these APIs and if we can't do that (for > > example because the expression can't be repeated with the same result), at > > least make that safer/easier to do. > > > I agree we need to test all API's, but the testing strategies can differ. I > am aware that "frame variable" and "expression" are very different. However, > SBFrame::EvaluateExpression is not that different from the "expression" > command -- in fact, I'd say that it's indistinguishable as for the stuff > you're interested in testing here. And same goes for "frame variable" and > SBFrame::GetValueForVariablePath. > > So, I think we could just use one of the two mechanisms for the general > evaluation test, and then have different tests for which test the unique > properties of the specific mechanisms (e.g., command line option parsing). I > certainly don't see a reason to run the same expression both through the > command line and SB apis "just in case". > > I'm thinking/hoping that the SB api approach would be a better fit for the > "run this expression and see what you get" kinds of tests, because it avoids > the need to reverse-engineer the values/types out of the textual output. I haven't compared how similar the actual implementations are, but moving that to just use the SB API seems ok. ================ 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: > 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. 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
