[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D100191#2692882 <https://reviews.llvm.org/D100191#2692882>, @mgorny wrote:

> In D100191#2689489 <https://reviews.llvm.org/D100191#2689489>, @labath wrote:
>
>> We should also start thinking about tests. I suppose the smallest piece of functionality that could be usefully tested (with a lldb-server test) is debugging a process that forks, stopping after the fork, and detaching from the child. Shall we try making that work first?
>
> How about using a `MockProcess` to unittest server's behavior wrt getting `NewSubprocess()` and `Detach()` calls?

I have mixed feelings about that. On one hand, these kinds of tests really enable you to test scenarios (often to do with various borderline and error conditions) which would not be testable in other ways. And they can run on any platform. However, you're not actually testing the error conditions, and I also have to put a bunch of other things on the other tip of the scale:

- this test is not a very good example of how tests should be written -- it doesn't test the public interface of the class, and it doesn't even set it up in a very realistic way. For example, the class does not have a connection object initialized. I'm guessing you cannot check the result of the `Handle_D` function (which is directly touched by this patch, and it is at least close to the public API), because it would always return an error due to a missing connection. All of this makes the test very fragile
- writing this test wouldn't "absolve" us of the need to write a test at a higher level, which would test the integration with a real process class. All of the checks you do here could be done at the lldb-server test level, and within the existing test framework (you couldn't check that `m_current_process` is null, but you could for example check that a `g` packet returns an error, which should be close enough).

So overall, I think that David was right that this patch is too small to be tested independently. Though I didn't say that explicitly, this is what I was also getting at with the "minimal usefully testable functionality" remark. So, while I hate to waste the effort you put into this test (and I'm very sorry that my slow response made you waste it), I don't think it would be wise to waste more of it in trying to make the test look good.

Let's identify the set of patches needed to make this testable via the lldb-server suite (this one, D100153 <https://reviews.llvm.org/D100153>, D100208 <https://reviews.llvm.org/D100208> (or equivalent for some other os), and what else?) and test that?



================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:226
+    NewSubprocess(NativeProcessProtocol *parent_process,
+                  std::unique_ptr<NativeProcessProtocol> &child_process) = 0;
   };
----------------
mgorny wrote:
> labath wrote:
> > That way, the delegate _must_ do something with the child process.
> Indeed, it must. Unfortunately, this breaks `MockDelegate`:
> 
> ```
> /home/mgorny/git/llvm-project/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h: In member function ‘virtual testing::internal::Function<void()>::Result lldb_private::MockDelegate::NewSubprocess(testing::internal::Function<void(lldb_private::NativeProcessProtocol*)>::Argument1, testing::internal::Function<void(lldb_private::NativeProcessProtocol*, std::unique_ptr<lldb_private::NativeProcessProtocol>)>::Argument2)’:
> /home/mgorny/git/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:405:73: error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = lldb_private::NativeProcessProtocol; _Dp = std::default_delete<lldb_private::NativeProcessProtocol>]’
>   405 |     return GMOCK_MOCKER_(2, constness, Method).Invoke(gmock_a1, gmock_a2); \
>       |                                                                         ^
> /home/mgorny/git/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:679:30: note: in expansion of macro ‘GMOCK_METHOD2_’
>   679 | #define MOCK_METHOD2(m, ...) GMOCK_METHOD2_(, , , m, __VA_ARGS__)
>       |                              ^~~~~~~~~~~~~~
> /home/mgorny/git/llvm-project/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h:28:3: note: in expansion of macro ‘MOCK_METHOD2’
>    28 |   MOCK_METHOD2(NewSubprocess,
>       |   ^~~~~~~~~~~~
> [...]
> ```
> 
> and then there are a few pages of errors.
Right. I see. In that case, I think we should fix the problem inside MockDelegate. The simplest way to do that is to add a forwarding method which repackages the arguments in a gmock-friendly way. Something like this ought to work:
```
class MockDelegate {
  MOCK_METHOD2(NewSubprocessImpl,
               void(NativeProcessProtocol *parent_process,
                    std::unique_ptr<NativeProcessProtocol> &child_proces /*or NativeProcessProtocol &, or whatever works best*/));
  void NewSubprocess(NativeProcessProtocol *parent_process,
                    std::unique_ptr<NativeProcessProtocol> child_proces) { NewSubprocessImpl(parent_process, child_process); }
};
```

That way, gmock is happy, and the code remains unaffected by testing artifacts.


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

https://reviews.llvm.org/D100191