Extended ProcessWrap with GetProcessById, GetCurrentProcess() and Kill methods #32
Conversation
| /// Gets a new System.Diagnostics.Process component and associates it with the currently active process. | ||
| /// </summary> | ||
| /// <returns>A new System.Diagnostics.Process component associated with the process resource that is running the calling application.</returns> | ||
| Process GetCurrentProcess(); |
There was a problem hiding this comment.
This should return the IProcess interface to allow proper mocking.
There was a problem hiding this comment.
Implementation of this method calls static Process.GetCurrentProcess which returns object of Process class. If we would like to return IProcess we would have to return here ProcessWrap instance or any other class implementing IProcess that would have to be created.
This would make the code a little bit more complicated. Although there are benefits for returning IProcess here, I believe that in majority of cases current implementation should be enough.
If someone would see benefits for himself having IProcess being returned I'm happy to implement it. For our needs it's enough to just return Process object
There was a problem hiding this comment.
The point of this library is test-ability of the code and easy mocking. If the return value cannot be mocked, there is no point in implementing this API in this library.
There was a problem hiding this comment.
Changed approach. I introduced IProcessFactory that is responsible for creating IProcess object. So basically all Process static methods are placed there.
Now we are able to return IProcess instead of Process
| /// </summary> | ||
| /// <param name="processId">The system-unique identifier of a process resource.</param> | ||
| /// <returns>A System.Diagnostics.Process component that is associated with the local process resource identified by the processId parameter.</returns> | ||
| Process GetProcessById(int processId); |
There was a problem hiding this comment.
This should return the IProcess interface to allow proper mocking.
| /// <param name="processId">The system-unique identifier of a process resource.</param> | ||
| /// <param name="machineName">The name of a computer on the network.</param> | ||
| /// <returns>A System.Diagnostics.Process component that is associated with a remote process resource identified by the processId parameter.</returns> | ||
| Process GetProcessById(int processId, string machineName); |
There was a problem hiding this comment.
This should return the IProcess interface to allow proper mocking.
|
Please, include also unit tests for newly implemented methods. |
|
Are there any plans to merge this? |
jozefizso
left a comment
jozefizso
left a comment
There was a problem hiding this comment.
Please, include unit and/or integration tests for newly implemented methods.
|
The code is missing tests so it wasn’t merged yet. |
3 participants
No description provided.