[Lldb-commits] [PATCH] D87172: Check if debug line sequences are starting after the first code segment

[Pavel Labath via Phabricator via lldb-commits]

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