This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Add expect_var_path to test variable path results
ClosedPublic

Authored by teemperor on Oct 30 2020, 3:21 AM.

Details

Summary

This adds expect_var_path to test variable paths so we no longer have to
use frame var and find substrs in the command output. The behaviour
is identical with expect_expr (and it also uses the same checking backend),
but it instead calls GetValueForVariablePath to evaluate the string as a variable
path.

Also rewrites a few of the tests that previously used frame variable to use
expect_var_path.

Diff Detail

Event Timeline

teemperor created this revision.Oct 30 2020, 3:21 AM
teemperor requested review of this revision.Oct 30 2020, 3:21 AM
labath accepted this revision.Oct 30 2020, 6:48 AM

Looks good to me. I was looking to find the SB equivalent of the "target variable" command (so we can use this without a running process, just like expect_expr), but I didn't find one. It would be good to add one some day...

lldb/packages/Python/lldbsuite/test/lldbtest.py
2595

Maybe it would be enough to call this expect_var ?

2598–2601

I'm wondering if the result_ prefixes are really needed. They would make the usage shorter, and it seems like the statements would flow just fine without it (expect_var[_path]("my_var", summary="..."))...

2613

assertEqual (and maybe drop the error message)

This revision is now accepted and ready to land.Oct 30 2020, 6:48 AM
teemperor marked 2 inline comments as done.Nov 5 2020, 3:28 AM
teemperor added inline comments.
lldb/packages/Python/lldbsuite/test/lldbtest.py
2595

expect_var sounds more like a function that can only check a plain variable (and IIRC we had some plans to have a dedicated function for this use case that uses both expr/frame var to access one variable). We could maybe do something like expect_frame_var to make the name more similar to what people know from the command line?

2598–2601

I think the original reason why the expect_expr had the result_ prefixes was that the variable names we use (or maybe will use for other Type/Value properties) in that function are easily colliding with builtin Python stuff (like, type technically works as an argument name but it shadows Python's builtin type function).

But we're anyway already using type in ValueCheck and it's really easier on the eyes, so I'll remove the prefixes.

labath added inline comments.Nov 5 2020, 4:35 AM
lldb/packages/Python/lldbsuite/test/lldbtest.py
2595

expect_var_path sounds about equally good/bad as expect_frame_var to me. I can see how expect_var may be a bit cryptic, though OTOH, these days we have "v" as an alias to "frame var"..

I don't have strong opinions either way...

teemperor updated this revision to Diff 303108.Nov 5 2020, 6:30 AM
teemperor marked an inline comment as done.
  • Addressed feedback
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2020, 7:15 AM