On Mon, 2007-06-11 at 01:05 -0700, James Hawkins wrote: > On 6/10/07, Misha Koshelev <[EMAIL PROTECTED]> wrote: > > Tested on WinXP and Win98 (and of course wine). > > --- > > dlls/setupapi/tests/Makefile.in | 1 + > > dlls/setupapi/tests/install.c | 152 > > +++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 153 insertions(+), 0 deletions(-) > > > > +static void test_install(LPCSTR filename) > > +{ > > + char cmdline[MAX_PATH * 2]; > > + LONG ret; > > + > > + sprintf(cmdline, "DefaultInstall 128 %s\\%s", CURR_DIR, filename); > > + > > + if (useW == -1) > > + { > > + HKEY hkey; > > + useW = 1; > > + _InstallHinfSection(NULL, NULL, cmdline, 0); > > + if (RegOpenKey(HKEY_CURRENT_USER, > "Software\\Wine\\setupapitest", &hkey) == ERROR_SUCCESS) > > + RegCloseKey(hkey); > > + else > > + { > > + skip("InstallHinfSectionW is broken, using > InstallHinfSectionA\n"); > > + useW = 0; > > + _InstallHinfSection(NULL, NULL, cmdline, 0); > > + } > > + } > > + else _InstallHinfSection(NULL, NULL, cmdline, 0); > > + > > + ret = RegDeleteKey(HKEY_CURRENT_USER, "Software\\Wine\\setupapitest"); > > + ok(ret == ERROR_SUCCESS, "Expected registry key > Software\\Wine\\setupapitest to exist, RegDeleteKeyA returned %d\n", > ret); > > +} > > + > > +static void test_InstallHinfSection(void) > > +{ > > + create_inf_file(inffile); > > + test_install(inffile); > > + ok(DeleteFile(inffile), "Expected source inf to exist, last error > was %d\n", GetLastError()); > > +} > > This would be much simpler if you didn't use the extra test_install > function. What is the point of that? Unit test functions should be > atomic. Each test function should create the files it needs, and make > sure the files it created are cleaned up by the end of the function > (replace files with registry keys, etc). You can also look at it like > this: you should be able to remove test function x without affecting > any other tests, which is not the case here. The check for DeleteFile > would obviously fail. Unit tests should be clear and easy to > understand what's being tested, and honestly, this code confused me on > my first reading. I will rename test_install, I think the test_ naming of it is confusing. It is just another helper function, and the point is more clear in patch 2 I think (where I use a space in the path and show that it doesn't work on our implementation).
> > + _InstallHinfSection(NULL, NULL, cmdline, 0); > > Why aren't you checking the return value, or GetLastError? Both are void functions. I can check for GetLastError() actually working for this function but it is not documented in msdn to do so. > > + skip("InstallHinfSectionW is broken, using > InstallHinfSectionA\n"); > > How is it broken? This needs to be clarified with a comment. You > really should just test only the A function in the first place though. Yes, that is what I tried first just using the A function. However, on Windows XP the functionality of this function is that it returns and does not do anything. I thought something was wrong with my code at first, but the inf file being generated was actually installed just fine from the command line, and I found a comment on a Russian language programming forum with someone who had the same problem and just recommended switching to the W function. I switched to W and it worked fine with no other changes. Similary, on Win95, the W function exists but seems to have this functionality of not doing anything ,whereas the A function works perfectly with the exact same inf and cmdline. I will add a more detailed comment about this. > > + if (pInstallHinfSectionW && !pInstallHinfSectionA) useW = 1; > > Is there ever a time when InstallHinfSectionA doesn't exist? If it > doesn't exist, you're going to crash if InstallHinfSectionW is > 'broken': This case is taken care of in the TEST(install) section (actually all cases of functions not existing). useW only stays -1 if both functions exist. > > + skip("InstallHinfSectionW is broken, using > InstallHinfSectionA\n"); > > + useW = 0; > > + _InstallHinfSection(NULL, NULL, cmdline, 0); > Misha