Re: winspool: Add a test for DeviceCapabilities, fix some bugs found

[Dmitry Timoshkov]

"Detlef Riekenberg"  wrote:
> > - hComdlg32 = LoadLibrary("comdlg32.dll");
> > - assert(hComdlg32);
> > - pPrintDlg = GetProcAddress(hComdlg32, "PrintDlgA");
> > - assert(pPrintDlg);
>
> Why are you using assert() and kill the test?
> Is skip not enough?

Skip is supposed to skip tests for optional functionality that doesn't
present on some systems, that's not the case here.
> > - ok(dm != NULL, "GlobalLock(prn_dlg.hDevMode) failed\n");
> > - trace("dmDeviceName "%s"\n", dm->dmDeviceName);
>
> You added a ok() for "dm =! NULL", but "dm" is accessed always and will
> crash the test.
> > - ok(dn != NULL, "GlobalLock(prn_dlg.hDevNames) failed\n");
> > - ok(dn->wDriverOffset, "expected not 0 wDriverOffset\n");
>
> Same here for "dn".

That's the point of the test. dm and dn must not be NULL in all cases.
> > - ok(lstrcmp((const char *)dm->dmDeviceName, (const char *)dn + dn->wDeviceOffset) == 0, "device names not
> >
> > match\n");
>
> You introduced the same bug, as you did in in your previous Patch!
> (test_DocumentProperties)
>
> dm->dmDeviceName is limited to CCHDEVICENAME

I don't see a problem here.
> > - trace("fields %x\n", fields);
> >
> > +todo_wine
> > - ok(fields == dm->dmFields, "fields %x != dmFields %x\n", fields, dm->dmFields);
>
> info.c:2093:fields 7b13
> info.c:2095: Test succeeded inside todo block: fields 7b13 != dmFields
> 7b13
>
> I get the same "succeeded inside todo" for all printers

I have only 1 configured printer here: "Wine Postscript Driver", and the test
fails for me.
> >  test_SetDefaultPrinter();
> >  test_XcvDataW_MonitorUI();
> >  test_XcvDataW_PortIsValid();
> > - test_DeviceCapabilities();
>
> Why is it so difficult to add the tests in alphabetic order?

Some tests may depend on the results of other tests, or cause side effects,
so it's always good to add the tests at the end of the sequence to not cause
any possible problem. Feel free to send a patch and re-arrange the tests.
> This Patch dump a lot of unneeded data.
> Please remove the traces (or dump the data only in interactive mode).

The traces are supposed to help understanding what's wrong with the test when
somebody looks at the test results at test.winehq.org and trys to figure out
how to fix a failing test.
> You are testing "PrintDlgA" in the testsuite for "winspool.drv".
> The correct location is: dlls/comdlg32/tests/printdlg.c

The test exercises the functionality of winspool, but does that with the help
of PrintDlgA to avoid code duplication. The test doesn't add a hard dependency
on comdlg32.dll, so I don't see a problem.

Thanks for the review.