Re: winspool/tests: New Testsuite

[Detlef Riekenberg]

New Testsuite for GetDefaultPrinter and GetPrinterDriverDirectory

Added:
+ Test Printing Environments for GetPrinterDriverDirectory
+ More intensive Testing.
+ Verbose Output with "WINETEST_DEBUG" > 1

* More tests will follow.

If the Patch is to Large (i already removed the Unicode-Tests)
i can split the Testsuite, but this will reduce the amount of testing,
compared to the current CVS.

Changelog:
 - New Testsuite for winspool.drv:
   GetDefaultPrinterA and
   GetPrinterDriverDirectoryA (with different environments)
 - Verbose Output with "WINETEST_DEBUG" > 1

winspool/tests: New Testsuite

[Vitaliy Margolen]

Sunday, September 25, 2005, 5:44:36 PM, Detlef Riekenberg wrote:
> If the Patch is to Large (i already removed the Unicode-Tests)
> i can split the Testsuite, but this will reduce the amount of testing,
> compared to the current CVS.

No kidding it's so large! You can shrink it down to probably 10% of it's current
size.
> -
>
> +/* ############################
> -
> -
> - */
> -

Please remove these lines.
> - if(opt_verbose)
> - {
> -
>    trace("-----\n");
> - }
> -

This is not really informative to print it at all.
> - buffer = HeapAlloc(GetProcessHeap(), 0, size);
> - lasterror=GetLastError();
> - if(opt_verbose)
> - {
> -
>    trace("%p: HeapAlloc(%p, 0, 0x%lx/%ld) => (0x%lx/%ld)\n", buffer,
> -
>            GetProcessHeap(), size, size, lasterror, lasterror);
> -
> -
>    put_lasterror();
> - }
> - ok(buffer != NULL, "allocating 0x%ld/%ld Bytes for Buffer => (0x%lx/%ld)\n",
> -
>            size, size, lasterror, lasterror);

Will look  better like this:
        buffer = HeapAlloc(GetProcessHeap(), 0, size);
        ok(buffer != NULL, "HeapAlloc(%ld) error=%lx\n", size, GetLastError());
> - if(pGetPrinterDriverDirectoryA == NULL)
> - {
> -
>    return ;
> - }
> -

Looks much better this way:
        if(pGetPrinterDriverDirectoryA == NULL) return;
> - }else
> - {

Wine uses this style please follow it:
        if ()
        {
        }
        else
        {
        }
> - is_result_ok(FALSE);

Is not helpfull really. Indicate what failed. Adding extra verbose traces makes
output too big and hard to find what really failed.
> +#define ENVIRONMENT_NULL      0
> +#define ENVIRONMENT_WIN40     1
> +#define ENVIRONMENT_W32X86    2
> +#define ENVIRONMENT_IA64      3
> +#define ENVIRONMENT_W32ALPHA  4
> +#define ENVIRONMENT_W32PPC    5
> +#define ENVIRONMENT_W32MIPS   6
> +#define ENVIRONMENT_MAX       ENVIRONMENT_W32MIPS

Please look inside include/wine/test.h and other wine test files. And don't
invent platform detection. It's already done just use it. Or don't define
something that you don't need.

Most of the stuff that you added to dlls/winspool/tests/info.c is useless.

Vitaliy

winspool/tests: New Testsuite (coding style)

[Detlef Riekenberg]

Am Sonntag, den 25.09.2005, 19:17 -0600 schrieb Vitaliy Margolen:
> No kidding it's so large! You can shrink it down to probably 10% of it's current
> size.

We will see, what's left after rework.
> > +/* ############################

A Separator makes the source more readable but i try to reduce that.
> > - if(opt_verbose)
> > - {
> > -
> >    trace("-----\n");
> > - }
> This is not really informative to print it at all.

Accepted.
> > - buffer = HeapAlloc(GetProcessHeap(), 0, size);
> > - lasterror=GetLastError();
> > - if(opt_verbose)
> > - {
> > -
> >    trace("%p: HeapAlloc(%p, 0, 0x%lx/%ld) => (0x%lx/%ld)\n", buffer,
> > -
> >            GetProcessHeap(), size, size, lasterror, lasterror);
> > -
> > -
> >    put_lasterror();
> > - }
> > - ok(buffer != NULL, "allocating 0x%ld/%ld Bytes for Buffer => (0x%lx/%ld)\n",
> > -
> >            size, size, lasterror, lasterror);
> Will look  better like this:
>         buffer = HeapAlloc(GetProcessHeap(), 0, size);
>         ok(buffer != NULL, "HeapAlloc(%ld) error=%lx\n", size, GetLastError());

In this case, GetlastError() inside ok() works, but most times
i must take care, that NtCurrentTeb()->LastErrorValue is never
changed by any function called during a trace() / ok().
For this Reasons, i decided to cache the Result of GetLastError() in my
code.

My goal was to have the same style for every test,
but I change it to be more "K.I.S.S".

Since i want to test as much as possible, I test for:
- the returncode,
- then lasterror 
- and then the returned data.

This is needed, because we have many bugs in all areas.
Example:
- Define a printer in linux.
- Run wine with an app that uses winspool.drv
- Delete your Linux-Printer
- winspool.drv in wine will report now:
  * EnumPrinters => 0: correct,
  * GetDefaultPrinter => your deleted printer: this is wrong
> Looks much better this way:
>         if(pGetPrinterDriverDirectoryA == NULL) return;
> Wine uses this style please follow it:
>         if ()
>         {
>         }
>         else
>         {
>         }

Really?
cvs from yesterday:
grep -R "}.*else" * | wc -l
4402
grep -R " * else" * | grep -v "}.*else" | wc -l
10672

I can change that, but then we have:
a: if (check) function;

b: if (check) {
      function1;
      function2;
   }

c: if (check)
   {
      function_true;
   }
   else
   {
      function_false;
   }

No common Style.

I learned the Style i used in my Patch from the Editor "jfe", because
jfe did Autoident and other things with that Style.

I try to change that in my Patches, but "not fall back" to my old Style
is difficult .
> > - is_result_ok(FALSE);
>
> Is not helpfull really. Indicate what failed. Adding extra verbose traces makes
> output too big and hard to find what really failed.

I will do a deep view to this..
> > +#define ENVIRONMENT_NULL      0
> > +#define ENVIRONMENT_WIN40     1
> > +#define ENVIRONMENT_W32X86    2
> > +#define ENVIRONMENT_IA64      3
> > +#define ENVIRONMENT_W32ALPHA  4
> > +#define ENVIRONMENT_W32PPC    5
> > +#define ENVIRONMENT_W32MIPS   6
> > +#define ENVIRONMENT_MAX       ENVIRONMENT_W32MIPS
>
> Please look inside include/wine/test.h and other wine test files. And don't
> invent platform detection. It's already done just use it. Or don't define
> something that you don't need.

The Printing-Environments are defined my Microsoft.
"%SystemRoot%\system32\spool\drivers" and
"HKLM\System\CurrentControlSet\Control\Print\Environments"
are your friends :-)
PS: Our Registry-Entries for the driver is incomplete and wrong
> And don't invent platform detection. It's already done just use it.

Did you mean "sys_isNT" with this?
When a ok() is going to fail, we must make sure that this is really a
Failure on the Tested System.

Example: GetDefaultPrinterW is present, but not implemented
This is a failure on NT but ok on win9x.
A simple if() that find an unimplemented "GetDefaultPrinterW" could just
decide to return from the test_function but my way find more errors.
(I knew, that this is not perfect: With "Services for Unicode" on win9x,
no failure is allowed).
> Most of the stuff that you added to dlls/winspool/tests/info.c is useless.

I do not want to duplicate Code in the Testfile, so i concentrate the
common Helper-Functions in one File.
Since every Sourcefile must include the Filename as test name and
"info.c" was present before, i used that.

I removed many tests in different Files and the "not yet needed"
Helper-Functions from "info.c" to reduce the size of the Patch.