[Lldb-commits] [PATCH] D60300: [testsuite] Split Objective-C data formatter

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Looks fine to me, with some inline ideas you can implement if you think they're worthwhile.

Also, since you're looking at reducing test time, the question you can ask yourself is: is it really worth it running separate dwarf+dsym flavours of these tests? I'm of the view that this should not be necessary, as the data formatters operate on high level abstractions, which should hide any differences in the debug info format (and the fact that the debug info parsers generate correct and consistent abstractions should be tested elsewhere). However, I don't have a horse in this race, so the decision is entirely up to you...



================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCCF.py:21-23
+    def setUp(self):
+        # Call super's setUp().
+        ObjCDataFormatterTestCase.setUp(self)
----------------
I'm pretty sure these are unneeded, as all you do is call the overridden method.


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCCF.py:29-32
+        self.cf_data_formatter_commands()
+
+    def cf_data_formatter_commands(self):
+        """Test formatters for Core OSX frameworks."""
----------------
I think this pattern is a relict from the days when we didn't have the magic test multiplicator. In those days, tests were written as:
```
def test_foo_dsym(self):
  self.buildDsym()
  self.foo()

def test_foo_dwarf(self):
  self.buildDwarf()
  self.foo()

def foo(self):
  # do the real stuff here
```

However, now that we can generate test flavours automatically, I don't think it makes much sense. So, unless you find it useful for some reason, I think we can just remove the helper function (by deleting these four lines).


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCCF.py:45-53
+        # This is the function to remove the custom formats in order to have a
+        # clean slate for the next test case.
+        def cleanup():
+            self.runCmd('type format clear', check=False)
+            self.runCmd('type summary clear', check=False)
+            self.runCmd('type synth clear', check=False)
+
----------------
It doesn't look like this test is doing anything to the type summaries, so I don't think it's necessary to clean up anything here.


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

https://reviews.llvm.org/D60300