Re: [1/2] shell32/tests: Trace the last ShellExecute command whenever a corresponding test fails.
Francois Gouget fgouget@codeweavers.com writes:
Signed-off-by: Francois Gouget fgouget@codeweavers.com
dlls/shell32/tests/shlexec.c | 75 ++++++++++++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 23 deletions(-)
diff --git a/dlls/shell32/tests/shlexec.c b/dlls/shell32/tests/shlexec.c index 8f03799..360fb2b 100644 --- a/dlls/shell32/tests/shlexec.c +++ b/dlls/shell32/tests/shlexec.c @@ -94,6 +94,28 @@ static int _todo_wait = 0; #define todo_wait for (_todo_wait = 1; _todo_wait; _todo_wait = 0)
static char shell_call[2048]=""; +static char last_shell_call[2048]=""; +static void WINETEST_PRINTF_ATTR(2,3) _okShell(int condition, const char *msg, ...) +{
- va_list valist;
- /* Note: if winetest_debug > 1 the ShellExecute() command has already been
* traced.*/- if (winetest_debug <= 1 && !condition && strcmp(last_shell_call, shell_call))
- {
winetest_trace("Called %s\n", shell_call);strcpy(last_shell_call, shell_call);- }
- va_start(valist, msg);
- winetest_vok(condition, msg, valist);
- va_end(valist);
+}
Why is that better than always printing it in the failing calls?
On Fri, 22 Jan 2016, Alexandre Julliard wrote:
Francois Gouget fgouget@codeweavers.com writes:
Signed-off-by: Francois Gouget fgouget@codeweavers.com
dlls/shell32/tests/shlexec.c | 75 ++++++++++++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 23 deletions(-)
diff --git a/dlls/shell32/tests/shlexec.c b/dlls/shell32/tests/shlexec.c index 8f03799..360fb2b 100644 --- a/dlls/shell32/tests/shlexec.c +++ b/dlls/shell32/tests/shlexec.c @@ -94,6 +94,28 @@ static int _todo_wait = 0; #define todo_wait for (_todo_wait = 1; _todo_wait; _todo_wait = 0)
static char shell_call[2048]=""; +static char last_shell_call[2048]=""; +static void WINETEST_PRINTF_ATTR(2,3) _okShell(int condition, const char *msg, ...) +{
- va_list valist;
- /* Note: if winetest_debug > 1 the ShellExecute() command has already been
* traced.*/- if (winetest_debug <= 1 && !condition && strcmp(last_shell_call, shell_call))
- {
winetest_trace("Called %s\n", shell_call);strcpy(last_shell_call, shell_call);- }
- va_start(valist, msg);
- winetest_vok(condition, msg, valist);
- va_end(valist);
+}
Why is that better than always printing it in the failing calls?
Because usually there is more than one failing call: there's the ok() after the shell_execute() and then multiple okChildXxx() calls to verify the data received by the child and there is often multiple failures. There is no reason for each of them to repeat the information about what ShellExecute() call failed. By comparing shell_call and last_shell_call okShell ensures that information is printed once before the series of failing tests.
It also makes it easier to ensure that this information is not duplicated when running with WINETEST_DEBUG > 1 since in that case shell_execute{_ex}() already traces the ShellExecute() call.
Another approach would be to systematically print the ShellExecute() call in shell_execute{_ex}() before we know if it fails or not. But that would only make the already large logs larger for no good reason.
Francois Gouget fgouget@codeweavers.com writes:
On Fri, 22 Jan 2016, Alexandre Julliard wrote:
Why is that better than always printing it in the failing calls?
Because usually there is more than one failing call: there's the ok() after the shell_execute() and then multiple okChildXxx() calls to verify the data received by the child and there is often multiple failures. There is no reason for each of them to repeat the information about what ShellExecute() call failed. By comparing shell_call and last_shell_call okShell ensures that information is printed once before the series of failing tests.
OTOH, it means that in silent mode you get no indication at all. Given that the checks are not supposed to fail, I'm not sure that improving the output for multiple failures is that important.
On Fri, 22 Jan 2016, Alexandre Julliard wrote:
Francois Gouget fgouget@codeweavers.com writes:
On Fri, 22 Jan 2016, Alexandre Julliard wrote:
Why is that better than always printing it in the failing calls?
Because usually there is more than one failing call: there's the ok() after the shell_execute() and then multiple okChildXxx() calls to verify the data received by the child and there is often multiple failures. There is no reason for each of them to repeat the information about what ShellExecute() call failed. By comparing shell_call and last_shell_call okShell ensures that information is printed once before the series of failing tests.
OTOH, it means that in silent mode you get no indication at all.
??? That's what we want right?
We don't want tests that succeed to spew out thousands of lines of traces because that would just bloat the logs, causing them to be rejected by test.winehq.org.
Given that the checks are not supposed to fail, I'm not sure that improving the output for multiple failures is that important.
Sure as long as you never add any new test and only run it on platforms where there are no failures then you don't get any failures and failure logging does not matter one bit.
But the ShellExecute() tests are very incomplete and I'm adding a bunch of new ones. And that's much easier to do if I get useful logs when the new test I add fails because Windows does not behave the way I expect.
Besides, even if we never add any new test we will get failures. All it takes is a new Windows version like Windows 10.
Francois Gouget fgouget@codeweavers.com writes:
On Fri, 22 Jan 2016, Alexandre Julliard wrote:
OTOH, it means that in silent mode you get no indication at all.
??? That's what we want right?
We don't want tests that succeed to spew out thousands of lines of traces because that would just bloat the logs, causing them to be rejected by test.winehq.org.
On failure you should be able to see what caused the failure, even in silent mode. With your change you'd have to re-run it with debug on, and no guarantee that you can get the same failure to happen again.
Sure as long as you never add any new test and only run it on platforms where there are no failures then you don't get any failures and failure logging does not matter one bit.
But the ShellExecute() tests are very incomplete and I'm adding a bunch of new ones. And that's much easier to do if I get useful logs when the new test I add fails because Windows does not behave the way I expect.
Besides, even if we never add any new test we will get failures. All it takes is a new Windows version like Windows 10.
Of course, I'm not arguing for not logging failures. On the contrary, my argument is that there's no reason to go out of your way to reduce output on failure (as opposed to output on success, which I agree should be minimized).
On Fri, 22 Jan 2016, Alexandre Julliard wrote:
Francois Gouget fgouget@codeweavers.com writes:
On Fri, 22 Jan 2016, Alexandre Julliard wrote:
OTOH, it means that in silent mode you get no indication at all.
??? That's what we want right?
We don't want tests that succeed to spew out thousands of lines of traces because that would just bloat the logs, causing them to be rejected by test.winehq.org.
On failure you should be able to see what caused the failure, even in silent mode. With your change you'd have to re-run it with debug on, and no guarantee that you can get the same failure to happen again.
Oh, I see, in the WINETEST_DEBUG=0 case winetest_trace() does not print anything. I never run in that mode (bug the testbot and WineTest do) so I missed that. I will send an updated patch.
[...]
Besides, even if we never add any new test we will get failures. All it takes is a new Windows version like Windows 10.
Of course, I'm not arguing for not logging failures. On the contrary, my argument is that there's no reason to go out of your way to reduce output on failure (as opposed to output on success, which I agree should be minimized).
It also makes the test failures more readable and has the benefit of encapsulating shell_call which is much better code-wise.
-
Alexandre Julliard -
Francois Gouget