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

[Detlef Riekenberg]

On Di, 2007-04-03 at 23:40 +0900, Dmitry Timoshkov wrote:
> Changelog:
>     winspool: Add a test for DeviceCapabilities, fix some bugs found.
> +    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?
> +    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".
> +    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
> +    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
> test_SetDefaultPrinter();
>      test_XcvDataW_MonitorUI();
>      test_XcvDataW_PortIsValid();
> +    test_DeviceCapabilities();

Why is it so difficult to add the tests in alphabetic order? 


This Patch dump a lot of unneeded data.
Please remove the traces (or dump the data only in interactive mode).


Thanks


-- 
 
By by ... Detlef

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

[Dmitry Timoshkov]

"Detlef Riekenberg" <wine.dev(a)web.de> 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.

-- 
Dmitry.

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

[Detlef Riekenberg]

On Mi, 2007-04-04 at 11:06 +0900, Dmitry Timoshkov wrote:
> "Detlef Riekenberg" <wine.dev(a)web.de> 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.

You introduced the same bug, as you did in in your previous Patch for
"winspool.drv", which was commited on 15. April 2006 
(test_DocumentProperties and test_DEVMODE)

Example failure for "Microsoft Office Document Image Writer":
http://test.winehq.org/data/200605051000/xp_RAYEN/winspool.drv:info.txt

More Examples for failing tests:
http://test.winehq.org/data/200605051000/#XP
http://test.winehq.org/data/200604231000/#XP
> > 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.

I use wineps.drv (WINEPS Printer using CUPS), but the drivers have the
same name as the cups-printer (patch from Huw a while ago).
My current cups-Printers are "cups-pdf" and 2 Parallel-Printers.
> > 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,

In this case, the test is broken and need to be fixed
(similar to the side-effects, that Paul fixed for advapi32)
> Feel free to send a patch and re-arrange the tests.
> You are able to fix it: Please fix it.
> > 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.

Dump additional data, when a test failed is ok, 
but in your Patch, the data is always dumped.
> > 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.

msi/tests use DeleteFile, but you would never add tests for DeleteFile
in the testsuite for msi.

Please add the tests related to PrintDlgA in
dlls/comdlg32/tests/printdlg.c

Thanks

-- 
 
By by ... Detlef