Page MenuHomePhabricator

[Dexter] Implement DexDeclareFile, a new Dexter command
Needs RevisionPublic

Authored by TWeaver on Mar 31 2021, 4:37 AM.

Details

Summary

DexDeclareFile allows test producers to write test files with .dex extensions that
contain pure dexter commands.

.dex file commands do not need to be commented out like they do when written
inline within test source files.

DexDeclareFile commands are declarative in behaviour, they state that any Dexter
command seen from this point on will have its path attribute set to the path
declared in the DexDeclareFile command.

Provided in this patch is in the implementation of DexDeclareFile and 4 tests.

Diff Detail

Event Timeline

TWeaver requested review of this revision.Mar 31 2021, 4:37 AM
TWeaver created this revision.

LGTM with a few tiny nits, but please wait for a +1 from someone else too.

debuginfo-tests/dexter/Commands.md
229

Changes -> Change (or maybe Set) fits the existing style of this document better imo, but this isn't very important.

debuginfo-tests/dexter/dex/command/ParseCommand.py
264

Should cmd_path get normalized with os.path.normcase here since the user can spell the path name in a few different ways on windows? Unhelpfully I can't remember exactly why it was, but when tinkering with this patch, at one point I did have to make that change.

Does everything work if the DexDeclareFile commands use\\annOying/PathSpelling (i.e. a mix of case and slashes that doesn't match the "canonical" spelling)? Maybe this deserves a test.

debuginfo-tests/dexter/dex/tools/TestToolBase.py
103

Tiny nit: comments should probably follow the llvm style (comments added here don't start with capitals but should).

jmorse accepted this revision.Apr 6 2021, 9:45 AM

LGTM, with a couple of nits, plus Orlandos comments should be addressed.

The precompiled binary test is great, that should get us going in the right direction.

debuginfo-tests/dexter/dex/tools/test/Tool.py
144–145

Mega nit: self.context.options.source_files.extend(list(new_source_files)) is slightly more concise.

debuginfo-tests/dexter/feature_tests/commands/penalty/dex_declare_file.cpp
16

Major nit, but I think this should be a filename that absolutely screams that it doesn't exist; thus something like "this_file_should_not_exist.cpp". that avoids the reader spending a second wondering whether the "not" prefix is significant.

(This might be because my default name for anything, when moved, is not_$originalname).

This revision is now accepted and ready to land.Apr 6 2021, 9:45 AM
jmorse requested changes to this revision.Apr 6 2021, 2:54 PM

I ended up taking this for a test drive, and found some difficulty when using the LLDB bindings -- _add_conditional_breakpoint ends up being passed a PurePath object for the file_, which is then passed to the LLDB bindings, which croak. This is easily fixed by calling str(file_) to convert the PurePath object to a string, but this might affect other debugger drivers & need some assertions adding.

This revision now requires changes to proceed.Apr 6 2021, 2:54 PM
Orlando added inline comments.Apr 8 2021, 9:41 AM
debuginfo-tests/dexter/feature_tests/commands/perfect/dex_declare_file/dex_and_source/test.cpp
9

This lit test fails for me locally - dexter doesn't run the test. Adding a test.cfg file to this directory fixes it.

TWeaver marked 5 inline comments as done.Mon, Apr 19, 9:11 AM
TWeaver added inline comments.
debuginfo-tests/dexter/feature_tests/commands/perfect/dex_declare_file/dex_and_source/test.cpp
9

slightly perplexing, this patch contains the test.cfg for the directory in question...

Orlando added inline comments.Tue, Apr 20, 1:14 AM
debuginfo-tests/dexter/feature_tests/commands/perfect/dex_declare_file/dex_and_source/test.cpp
9

Ah yes I see it, I must've messed up applying the patch and not noticed. Sorry!