Re: [PATCH 01/2] dpnet/test: Share code between tests (resend)
On Mon, Jan 30, 2017 at 12:05 AM, Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> wrote:
Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/dpnet/tests/address.c | 28 +------- dlls/dpnet/tests/client.c | 150 +-------------------------------------- dlls/dpnet/tests/dpnet_private.h | 32 +++++++++ dlls/dpnet/tests/server.c | 16 ++--- ...
I have never seen a "*_private.h" header in any of the other tests, though there are a small number of "*_test.h" headers. This instance might be _especially_ confusing since it would result in both a "./dlls/dpnet/dpnet_private.h" and a "./dlls/dpnet/tests/dpnet_private.h". The second patch looks reasonable to me, though you may wish for others to weigh in. Best, Erich
Hi Erich, On 07/02/17 15:39, Erich E. Hoover wrote:
On Mon, Jan 30, 2017 at 12:05 AM, Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> wrote:
Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/dpnet/tests/address.c | 28 +------- dlls/dpnet/tests/client.c | 150 +-------------------------------------- dlls/dpnet/tests/dpnet_private.h | 32 +++++++++ dlls/dpnet/tests/server.c | 16 ++--- ...
I have never seen a "*_private.h" header in any of the other tests, though there are a small number of "*_test.h" headers. This instance might be _especially_ confusing since it would result in both a "./dlls/dpnet/dpnet_private.h" and a "./dlls/dpnet/tests/dpnet_private.h". The second patch looks reasonable to me, though you may wish for others to weigh in.
Thanks for the review. I'm happy to rename the header if thats all is needed to get it committed. I'll assume dpnet_test.h is fine unless someone else states something different. Best Regards Alistair
On Tue, Feb 7, 2017 at 12:53 AM, Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> wrote:
... Thanks for the review. I'm happy to rename the header if thats all is needed to get it committed. I'll assume dpnet_test.h is fine unless someone else states something different.
No problem! That was the thing that really stood out to me, the consolidation you're doing looks like a really solid move (personally, I hate duplicated code). The only other thing that I noticed is that is_process_elevated(), is_firewall_enabled(), and set_firewall() are all shared with the rpcrt4 tests, so it's _possible_ for there to be a lot more consolidation. However, to my knowledge, we don't have a mechanism for sharing routines like that between multiple per-dll tests. If someone is aware of such a mechanism then that may be a way to make this even better. Best, Erich
participants (2)
-
Alistair Leslie-Hughes -
Erich E. Hoover