[Lldb-commits] [PATCH] D109906: [lldb] [test] Add unittest for DynamicRegisterInfo::Finalize()

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In general, I'd prefer to avoid adding new methods to the class under test, and save subclassing for the cases where it's impossible to do things otherwise (abstract class, visibility restrictions (although that often means you're not testing what you should be)). These things could live inside an gtest fixture (subclass of testing::Test), which could have a DynamicRegisterInfo member if needed.



================
Comment at: lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp:51-52
+                          std::vector<uint32_t> invalidate_regs = {}) {
+    std::string reg_msg =
+        llvm::formatv("at register {0} (num: {1})", reg_name, reg_num);
+    const RegisterInfo *reg = GetRegisterInfoAtIndex(reg_num);
----------------
Maybe use `SCOPED_TRACE` instead?


================
Comment at: lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp:61-93
+    if (value_regs.empty())
+      EXPECT_EQ(reg->value_regs, nullptr) << reg_msg;
+    else {
+      EXPECT_NE(reg->value_regs, nullptr) << reg_msg;
+
+      size_t i;
+      for (i = 0; i < value_regs.size(); i++) {
----------------
I'd probably write a helper function to convert the register lists to a vector, and then have gtest compare the vectors:
```
std::vector<uint32_t> to_vector(uint32_t *regs) {
  std::vector<uint32_t> result;
  if (regs) {
    while(*regs != LLDB_INVALID_REGNUM)
      result.push_back(*regs++);
  }
  return result;
}
...
ASSERT_THAT(to_vector(info->value_regs), value_regs);
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109906/new/

https://reviews.llvm.org/D109906