Re: [PATCH 1/3] shell32/tests: Add tests for FolderItems_Item and FolderItems_get_Count.
On 08.09.2017 8:29, Alex Henrie wrote:
- for (i = 0; i < sizeof(file_defs)/sizeof(file_defs[0]); i++)
- {
switch (file_defs[i].type){case DIRECTORY:r = CreateDirectoryA(file_defs[i].name, NULL);ok(r, "CreateDirectory failed: %08x\n", GetLastError());PathCombineA(buf, file_defs[i].name, "foo.txt");file = CreateFileA(buf, 0, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);ok(file != INVALID_HANDLE_VALUE, "CreateFile failed: %08x\n", GetLastError());CloseHandle(file);break;case EMPTY:file = CreateFileA(file_defs[i].name, 0, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);ok(file != INVALID_HANDLE_VALUE, "CreateFile failed: %08x\n", GetLastError());CloseHandle(file);break;case RANDOM:file = CreateFileA(file_defs[i].name, GENERIC_WRITE, 0, NULL,CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);ok(file != INVALID_HANDLE_VALUE, "CreateFile failed: %08x\n", GetLastError());for (j = 0; j < sizeof(buf); j++)buf[j] = rand() % 256;r = WriteFile(file, buf, sizeof(buf), &dwcount, NULL);ok(r, "WriteFile failed: %08x\n", GetLastError());CloseHandle(file);break;}- }
Why would it matter if file is empty or not? And if it does for some reason, why filling it with random content?
- MultiByteToWideChar(CP_ACP, 0, file_defs[0].name, -1, wstr, MAX_PATH);
- V_VT(&var) = VT_BSTR;
- V_BSTR(&var) = SysAllocString(wstr);
I understand why you have test names as single byte strings, but this construct is used more than once, so maybe add a helper for that. Could be similar to _bstr_() in msxml test, but without global pointer array.
- for (i = -10; i < 10; i++)
- {
V_VT(&var) = VT_ERROR;V_ERROR(&var) = i;
VT_ERROR is used for optional arguments, what does [-10,10) range mean? I think it's enough to test some HRESULT-style failure code.
ok(!!bstr, "bstr is null\n");ok(!lstrcmpW(bstr, cur_dir),"expected %s, got %s\n", wine_dbgstr_w(cur_dir), wine_dbgstr_w(bstr));
First check seems redundant.
Nikolay,
Thanks for the feedback. Before I dig into the shell32 code further, do you have any more thoughts about my msxml3 patch that has been sitting in the queue? I'd prefer to not be working on two DLLs at the same time with the same person.
-Alex
On 08.09.2017 18:28, Alex Henrie wrote:
Nikolay,
Thanks for the feedback. Before I dig into the shell32 code further, do you have any more thoughts about my msxml3 patch that has been sitting in the queue? I'd prefer to not be working on two DLLs at the same time with the same person.
I'm worried it could unexpectedly break more than it fixes. It's already fragile regarding whitespace normalization.
For example I see you still have todo for msxml6 case, where output differs. It might seem unimportant but unfortunately it is some applications (and servers) are picky about document structure. Do you think it's possible to fix that too?
-Alex
2017-09-08 9:53 GMT-06:00 Nikolay Sivov bunglehead@gmail.com:
On 08.09.2017 18:28, Alex Henrie wrote:
Nikolay,
Thanks for the feedback. Before I dig into the shell32 code further, do you have any more thoughts about my msxml3 patch that has been sitting in the queue? I'd prefer to not be working on two DLLs at the same time with the same person.
I'm worried it could unexpectedly break more than it fixes. It's already fragile regarding whitespace normalization.
For example I see you still have todo for msxml6 case, where output differs. It might seem unimportant but unfortunately it is some applications (and servers) are picky about document structure. Do you think it's possible to fix that too?
That todo is for a completely different bug. It was broken before my patch, and it's broken a little less after my patch.
Newline normalization is mandated by section 2.11 of the XML specification, so it should not come as a surprise to anyone: https://www.w3.org/TR/REC-xml/#sec-line-ends
If the only problem with the patch is a vague uneasiness about it, maybe you could agree to accept it if Wine Staging takes the patch and no bugs are reported within a couple of months.
-Alex
On 08.09.2017 19:06, Alex Henrie wrote:
2017-09-08 9:53 GMT-06:00 Nikolay Sivov bunglehead@gmail.com:
On 08.09.2017 18:28, Alex Henrie wrote:
Nikolay,
Thanks for the feedback. Before I dig into the shell32 code further, do you have any more thoughts about my msxml3 patch that has been sitting in the queue? I'd prefer to not be working on two DLLs at the same time with the same person.
I'm worried it could unexpectedly break more than it fixes. It's already fragile regarding whitespace normalization.
For example I see you still have todo for msxml6 case, where output differs. It might seem unimportant but unfortunately it is some applications (and servers) are picky about document structure. Do you think it's possible to fix that too?
That todo is for a completely different bug. It was broken before my patch, and it's broken a little less after my patch.
Ok.
Newline normalization is mandated by section 2.11 of the XML specification, so it should not come as a surprise to anyone: https://www.w3.org/TR/REC-xml/#sec-line-ends
If the only problem with the patch is a vague uneasiness about it, maybe you could agree to accept it if Wine Staging takes the patch and no bugs are reported within a couple of months.
I have no problem with that option.
-Alex
2017-09-08 2:12 GMT-06:00 Nikolay Sivov bunglehead@gmail.com:
On 08.09.2017 8:29, Alex Henrie wrote:
- for (i = 0; i < sizeof(file_defs)/sizeof(file_defs[0]); i++)
- {
switch (file_defs[i].type){case DIRECTORY:r = CreateDirectoryA(file_defs[i].name, NULL);ok(r, "CreateDirectory failed: %08x\n", GetLastError());PathCombineA(buf, file_defs[i].name, "foo.txt");file = CreateFileA(buf, 0, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);ok(file != INVALID_HANDLE_VALUE, "CreateFile failed: %08x\n", GetLastError());CloseHandle(file);break;case EMPTY:file = CreateFileA(file_defs[i].name, 0, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);ok(file != INVALID_HANDLE_VALUE, "CreateFile failed: %08x\n", GetLastError());CloseHandle(file);break;case RANDOM:file = CreateFileA(file_defs[i].name, GENERIC_WRITE, 0, NULL,CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);ok(file != INVALID_HANDLE_VALUE, "CreateFile failed: %08x\n", GetLastError());for (j = 0; j < sizeof(buf); j++)buf[j] = rand() % 256;r = WriteFile(file, buf, sizeof(buf), &dwcount, NULL);ok(r, "WriteFile failed: %08x\n", GetLastError());CloseHandle(file);break;}- }
Why would it matter if file is empty or not? And if it does for some reason, why filling it with random content?
I was originally going to add tests for shortcut files, and I wanted to see whether FolderItem_IsLink returns TRUE for a .lnk file that is not actually a shortcut. You're right that empty files would be good enough for these tests though.
- MultiByteToWideChar(CP_ACP, 0, file_defs[0].name, -1, wstr, MAX_PATH);
- V_VT(&var) = VT_BSTR;
- V_BSTR(&var) = SysAllocString(wstr);
I understand why you have test names as single byte strings, but this construct is used more than once, so maybe add a helper for that. Could be similar to _bstr_() in msxml test, but without global pointer array.
Sure, that sounds reasonable.
- for (i = -10; i < 10; i++)
- {
V_VT(&var) = VT_ERROR;V_ERROR(&var) = i;VT_ERROR is used for optional arguments, what does [-10,10) range mean? I think it's enough to test some HRESULT-style failure code.
Just trying to be thorough; VT_ERROR returns the parent folder regardless of the numeric value. Is it OK to only test 0?
ok(!!bstr, "bstr is null\n");ok(!lstrcmpW(bstr, cur_dir),"expected %s, got %s\n", wine_dbgstr_w(cur_dir), wine_dbgstr_w(bstr));First check seems redundant.
OK, I'll remove the null checks before string comparisons. Thanks again for the feedback.
-Alex
-
Alex Henrie -
Nikolay Sivov