Re: [PATCH] wine/test.h: Increment correct counter for tests in todo blocks

[Nikolay Sivov]

On 21.09.2017 19:11, Fabian Maurer wrote:
> Tests succeeding in todo blocks are counted as failures,
> and failures as success. This patch fixes that.
>
> Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de>
> ---
>  include/wine/test.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/wine/test.h b/include/wine/test.h
> index 4ec66d5391..f0ee617d5f 100644
> --- a/include/wine/test.h
> +++ b/include/wine/test.h
> @@ -311,7 +311,7 @@ int winetest_vok( int condition, const char *msg, __winetest_va_list args )
>              printf( "%s:%d: Test succeeded inside todo block: ",
>                      data->current_file, data->current_line );
>              vprintf(msg, args);
> -            InterlockedIncrement(&todo_failures);
> +            InterlockedIncrement(&todo_successes);
>              return 0;
>          }
>          else
> @@ -322,7 +322,7 @@ int winetest_vok( int condition, const char *msg, __winetest_va_list args )
>                          data->current_file, data->current_line );
>                  vprintf(msg, args);
>              }
> -            InterlockedIncrement(&todo_successes);
> +            InterlockedIncrement(&todo_failures);
>              return 1;
>          }
>      }

This is correct. If test passes but is marked as todo it is a failure.

[PATCH] wine/test.h: Increment correct counter for tests in todo blocks

[Fabian Maurer]

> If it succeeds inside a todo then it's counted as failure. If it fails
> as expected it's not counted.
> This is correct. If test passes but is marked as todo it is a failure.

I see, to detect if some test now works while it used to fail, I guess? Makes 
sense, but goes directly against the comments:
> static LONG todo_successes;  /* number of successful tests inside todo block
> static LONG todo_failures;   /* number of failures inside todo block */

According to this, it's the other way round. Or at least it's 
misunderstandable. But if it's working as intended, please ignore my patch.
But could we maybe rename these variables and change these comments so it's 
easier to understand?


Regards,
Fabian Maurer

[PATCH] wine/test.h: Increment correct counter for tests in todo blocks

[Nikolay Sivov]

On 21.09.2017 19:40, Fabian Maurer wrote:
> > If it succeeds inside a todo then it's counted as failure. If it fails
> > as expected it's not counted.
> > This is correct. If test passes but is marked as todo it is a failure.
>
> I see, to detect if some test now works while it used to fail, I guess? Makes 
> sense, but goes directly against the comments:

Right.
> > static LONG todo_successes;  /* number of successful tests inside todo block
> > static LONG todo_failures;   /* number of failures inside todo block */
>
> According to this, it's the other way round. Or at least it's 
> misunderstandable. But if it's working as intended, please ignore my patch.
> But could we maybe rename these variables and change these comments so it's 
> easier to understand?

todo_successes means todo block works, meaning current wine result
matches expected result negated by todo_wine. The fact that it matches
means todo block works as intended.
> Regards,
> Fabian Maurer