Re: [PATCH 1/3] setupapi/tests: Add rudimentary tests for InstallHinfSectionA/W.
On 6/10/07, Misha Koshelev <mk144210(a)bcm.edu> 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. + _InstallHinfSection(NULL, NULL, cmdline, 0); Why aren't you checking the return value, or GetLastError? + 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. + 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': + skip("InstallHinfSectionW is broken, using InstallHinfSectionA\n"); + useW = 0; + _InstallHinfSection(NULL, NULL, cmdline, 0); -- James Hawkins
On Mon, 2007-06-11 at 01:05 -0700, James Hawkins wrote:
On 6/10/07, Misha Koshelev <mk144210(a)bcm.edu> 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
On Mon, 2007-06-11 at 01:05 -0700, James Hawkins wrote:
On 6/10/07, Misha Koshelev <mk144210(a)bcm.edu> 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.
+ _InstallHinfSection(NULL, NULL, cmdline, 0);
Why aren't you checking the return value, or GetLastError?
+ 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.
+ 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': I meant START_TEST(install) is where this is taken care of, not TEST(install).
+ skip("InstallHinfSectionW is broken, using InstallHinfSectionA\n");
+ useW = 0;
+ _InstallHinfSection(NULL, NULL, cmdline, 0);
Misha
participants (2)
-
James Hawkins -
Misha Koshelev