labath added inline comments.
================
Comment at: lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp:349-369
+
+TEST_F(SymbolFileDWARFTests, EnsureLineEntriesExistInAfterFirstCodeSection) {
+ auto ExpectedFile = TestFile::fromYamlFile("test-invalid-addresses.yaml");
+ ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded());
+
+ lldb::ModuleSP module_sp =
+ std::make_shared<Module>(ExpectedFile->moduleSpec());
----------------
aadsm wrote:
> labath wrote:
> > I think this would be better as a shell test (if for nothing else, then
> > because other line table tests are written that way). `image lookup` is
> > pretty much a direct equivalent to `ResolveSymbolContext`, but its output
> > is more readable, and its easier to fiddle with these kinds of tests.
> I think what you describe is an end to end test but here I explicitly want to
> test the SymbolFileDWARF code because that's what I changed, it's more like a
> unit test.
> I would prefer to stay away from an end to end test for this particular
> change because 1) knowing that `image lookup` calls `ResolveSymbolContext` is
> an implementation detail 2) The output of `image lookup` could change over
> time, 3) Other bugs unrelated to this could be introduced that will make
> `image lookup` show a different content as well and fail the test.
On most days I am a proponent of unit tests, so I feel slightly odd for writing
this, but here it goes...
I think you'll find that a lot of people in the llvm community (which now has a
pretty big (but not as big as I'd like) intersection with the lldb community)
are not very enthusiastic about (c++) unit tests. They concede they have their
place, but are generally trying to avoid it. Some would rather create a brand
new command line tool wrapping an api (and then test that via lit) than to
write a unit test for it. Some effects of that can be seen in the design of the
lldb-test tool.
Although this approach, when taken to extremes, can be bad, I believe it has
it's advantages, for several reasons:
- changing the test does not require recompilation
- due to more people using it, more people are aware of how these tests work
(easier for them to inspect/modify it)
- for similar reasons, the infrastructure supporting these tests is better.
E.g., for the lit test, I can choose whether to generate the test binary via
yaml2obj, llvm-mc, clang, or something completely different. unit tests only
have yaml2obj available and for a long time it was implemented by shelling out
to the external binary.
Your points about the issues with lit tests are not untrue, but I wouldn't say
they are more true for this test than they are for any other of our lit tests,
so I don't see a need to treat this test specially.
It's definitely true that the lit test will be more broad that this unit test,
but it's still pretty far from a end-to-end test. A lot of our end-to-end tests
for dwarf features start by compiling some c++ codes (hoping it produces the
right dwarf), running the code, and then inspecting the running process. That's
something I'm trying to change (not very successfully). Compared to that,
"image lookup" is peanuts. Although you the fact that it's implemented by
`ResolveSymbolContext` is kind of an implementation detail, I would say that if
it was *not* implemented this way (and e.g. reimplemented some of that
functionality) that we have bigger problems.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87172/new/
https://reviews.llvm.org/D87172
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
