Re: 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.