On 2011-11-06 18:04, Vitaliy Margolen wrote:
On 11/06/2011 09:14 AM, Thomas Faber wrote:
+ length = strlen(dll_directories[i]); + assert(sizeof(buffer) > length + 1); Please don't do this. Tests shouldn't assert because programmer failed to pick the right buffer size of hard-coded strings.
Check.
+ ok(GetLastError() == 0xdeadbeef, "Error is %u\n", GetLastError()); You should get value of GetLastError() before the ok check here. See all places that check values of GetLastError().
Check.
+ ok(buffer[length + 1] == 'A', "i=%d, Buffer overflow\n", i); + ok(buffer[ret] == 0, "i=%d, Wrong length or not null terminated\n", i); + ok(strcmp(buffer, dll_directories[i]) == 0, "i=%d, Wrong path returned: '%s'\n", i, buffer); You can do all these with one memcmp call.
Indeed. That's rather a leftover from the other testcases.
+ ok(buffer[length + 1] == 'A', "i=%d, Buffer overflow\n", i); + ok(buffer[ret] == 0, "i=%d, Wrong length or not null terminated\n", i); + for (j = 0; buffer[j]; j++) + if (buffer[j] != dll_directories[i][j]) + { + ok(0, "i=%d, Wrong path returned: %s\n", i, wine_dbgstr_w(buffer)); + break; + } Same for this.
This one's rather tricky, as it's a comparison between an ansi and a unicode string (I didn't want to have the directories listed twice). I'll think of something nicer.
+ ok(buffer[0] == 0 || /* XP, 2003 */ + buffer[0] == 'A', "i=%d, Buffer overflow\n", i); Please mark bad case with broke() to check for correct Wine behavior.
Hmm, which one is broken though? Wine currently has XP's behavior, that is, put a null in there, even if the buffer size is zero. This is consistent with the A version of the function, but does seem less sensible.
Also please combine A & W tests. They do mostly the same things.
Agreed. Thanks for the review! I'll make an updated patch. -Tom