Page MenuHomePhabricator

[lldb] Add expect_expr function for testing expression evaluation in dotests.
ClosedPublic

Authored by teemperor on Nov 15 2019, 7:26 AM.

Details

Summary

This patch adds a new function to lldbtest: expect_expr. This function is supposed to replace the current approach
of calling expect/runCmd with expr, p etc.

expect_expr allows evaluating expressions and matching their value/summary/type/error message without
having to do any string matching that might allow unintended passes (e.g., self.expect("expr 3+4", substrs=["7"])
can unexpectedly pass for results like (Class7) $0 = 7, (int) $7 = 22, (int) $0 = 77 and so on).

This only uses the function in a few places to test and demonstrate it. I'll migrate the tests in follow up commits.

Diff Detail

Event Timeline

teemperor created this revision.Nov 15 2019, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2019, 7:26 AM

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)?

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. 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.

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. 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.

lldb/packages/Python/lldbsuite/test/lldbtest.py
2480–2482

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.

teemperor planned changes to this revision.Nov 18 2019, 4:43 AM
teemperor marked an inline comment as done.

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. 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.

lldb/packages/Python/lldbsuite/test/lldbtest.py
2480–2482

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.

labath added inline comments.Nov 18 2019, 5:52 AM
lldb/packages/Python/lldbsuite/test/lldbtest.py
2480–2482

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.

teemperor marked an inline comment as done.Nov 18 2019, 6:09 AM
teemperor added inline comments.
lldb/packages/Python/lldbsuite/test/lldbtest.py
2480–2482

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.

teemperor updated this revision to Diff 238189.Jan 15 2020, 1:04 AM
  • Moved to using the SBAPI.

We can't get fully rid of parsing some output as GetDescription of SBValue returns (type) $0 = VALUE\n but there
seems to be no way to get rid of the stuff around the value we want. But we now only strip it away instead of
trying to parse the type etc.

I also got rid of the expr->frame var->expr flow and it's not just expr->frame var. I don't want to remove them as we found two formatter bugs by testing both but once these thing don't break constantly then we can remove the 'expr' variant from the simple expression function.

teemperor updated this revision to Diff 238205.Jan 15 2020, 2:45 AM
  • Removed everything that is not summary or value.

This seems fine, assuming it is sufficient to achieve your goals.

lldb/packages/Python/lldbsuite/test/lldbtest.py
2396–2407

maybe something like self.expect(got, substrs=expected, exe=False) ?

2413

frame = self.frame(). If it turns out useful, we could also add an argument to run the expression on a specific frame...

teemperor updated this revision to Diff 238209.EditedJan 15 2020, 3:10 AM
  • Removed substr functionality.
  • Using frame() now.
teemperor marked 2 inline comments as done.Jan 15 2020, 3:12 AM
labath accepted this revision.Jan 15 2020, 3:16 AM

I think this is a great start. We can see how we can extend this later...

This revision is now accepted and ready to land.Jan 15 2020, 3:16 AM
teemperor retitled this revision from [lldb] Add better test commands for expression evaluation to [lldb] Add expect_expr function for testing expression evaluation in dotests..Jan 15 2020, 4:01 AM
teemperor edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.