Re: Re[2]: [PATCH] shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile (resend)

2010-02-12 Thread James Mckenzie
Ilya Basin  wrote at Feb 12, 2010 4:39 AM about Re[2]: 
[PATCH] shell32/tests: add tests for the parser of  SHELLEXECUTEINFO.lpFile 
(resend)
>
>Thanks for review. Not sending to wine-patches this time. New patch is
>in the bottom. What's better, to attach a generated patch or to use it
>as a message body?
>
Generated patch, if at all possible.  That way we can use git apply to install.

James McKenzie





Re[2]: [PATCH] shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile (resend)

2010-02-12 Thread Ilya Basin
Thanks for review. Not sending to wine-patches this time. New patch is
in the bottom. What's better, to attach a generated patch or to use it
as a message body?

PV> On 02/11/2010 03:37 PM, Ilya Basin wrote:
>> Hi! This should expose this bug:
>> http://bugs.winehq.org/show_bug.cgi?id=19385 ( the 'wine start'
>> launcher does not open MS Office documents that have spaces in their
>> path ). Although the title is misleading, many other tickets are
>> marked as duplicates of it.
>> It's not about spaces. It's about sometimes unneeded parsing of
>> SHELLEXECUTEINFO.lpFile, trying to extract arguments from it. Why?
>> Or maybe some other part of wine relies on this behavior?
>> Tested on 98 se, 2k&  xp.

PV> Hi Ilya,

PV> As Wine doesn't do the right thing (according to your tests), all 
PV> failing Wine tests should be marked as such. We have a todo_wine 
PV> construct for that.
added 4 todo_wine's

PV> We generally do not want any real version checking in the tests. Tests
PV> should decide based on behavior on what platform they are.
added GetProcAddress(hkernel32, "AttachConsole") to detect XP/2k3
removed special case when XP emulates 9x because can't do it without
using GetVersion().

>> +
>> +GetTempPathA(sizeof(fileA), fileA);
>> +shorttmpdir = tmpdir + strlen(fileA);
>> +
>> +/* ensure tmpdir is in %TEMP% */
>> +SetEnvironmentVariableA("TEMP", fileA);

PV> Why is this necessary?
/* ensure tmpdir is in %TEMP%: GetTempPath() can succeed even if TEMP is 
undefined */

>> +/* is SEE_MASK_DOENVSUBST default flag? Should only be when XP emulates 
>> 9x */
>> +{

PV> Any particular reason for this indentation?
OK

---
 dlls/shell32/tests/shlexec.c |   87 ++
 1 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/dlls/shell32/tests/shlexec.c b/dlls/shell32/tests/shlexec.c
index c673f0a..d575d22 100644
--- a/dlls/shell32/tests/shlexec.c
+++ b/dlls/shell32/tests/shlexec.c
@@ -797,6 +797,10 @@ static const char* testfiles[]=
 "%s\\test file.sde",
 "%s\\test file.exe",
 "%s\\test2.exe",
+"%s\\simple.shlexec",
+"%s\\drawback_file.noassoc",
+"%s\\drawback_file.noassoc foo.shlexec",
+"%s\\drawback_nonexist.noassoc foo.shlexec",
 NULL
 };
 
@@ -852,6 +856,88 @@ static filename_tests_t noquotes_tests[]=
 {NULL, NULL, 0}
 };
 
+static void test_lpFile_parsed(void)
+{
+/* basename tmpdir */
+const char* shorttmpdir;
+
+const char *testfile;
+char fileA[MAX_PATH];
+
+int rc;
+int expected;
+
+HMODULE hkernel32 = GetModuleHandle("kernel32");
+BOOL isreallyXP2k3plus = NULL != GetProcAddress(hkernel32, 
"AttachConsole");
+
+GetTempPathA(sizeof(fileA), fileA);
+shorttmpdir = tmpdir + strlen(fileA);
+
+/* ensure tmpdir is in %TEMP%: GetTempPath() can succeed even if TEMP is 
undefined */
+SetEnvironmentVariableA("TEMP", fileA);
+
+#define TEST_LPFILE_PARSED_OK_() (rc==expected || rc>32 && expected>32)
+#define TEST_LPFILE_PARSED_OK() ok(TEST_LPFILE_PARSED_OK_(), "expected %s 
(%d), got %s (%d), lpFile: %s \n", expected==33 ? "success" : "failure", 
expected, rc > 32 ? "success" : "failure", rc, fileA)
+
+/* existing "drawback_file.noassoc" prevents finding 
"drawback_file.noassoc foo.shlexec" on wine */
+testfile = "%s\\drawback_file.noassoc foo.shlexec";
+expected = 33; sprintf(fileA, testfile, tmpdir);
+rc=shell_execute(NULL, fileA, NULL, NULL);
+todo_wine { TEST_LPFILE_PARSED_OK(); }
+
+/* if quoted, existing "drawback_file.noassoc" not prevents finding 
"drawback_file.noassoc foo.shlexec" on wine */
+testfile = "\"%s\\drawback_file.noassoc foo.shlexec\"";
+expected = 33; sprintf(fileA, testfile, tmpdir);
+rc=shell_execute(NULL, fileA, NULL, NULL);
+TEST_LPFILE_PARSED_OK();
+
+/* error should be 2, not 31 */
+testfile = "\"%s\\drawback_file.noassoc\" foo.shlexec";
+expected = 2; sprintf(fileA, testfile, tmpdir);
+rc=shell_execute(NULL, fileA, NULL, NULL);
+TEST_LPFILE_PARSED_OK();
+
+/* ""command"" not works on wine (and real win9x and w2k) */
+if (isreallyXP2k3plus) {
+testfile = "\"\"%s\\simple.shlexec\"\"";
+expected = 33; sprintf(fileA, testfile, tmpdir);
+rc=shell_execute(NULL, fileA, NULL, NULL);
+todo_wine { TEST_LPFILE_PARSED_OK(); }
+} else {
+win_skip("ShellExecute('\"\"command\"\"', ...) only works on XP/2k3 or 
newer\n");
+}
+
+/* nonexisting "drawback_nonexist.noassoc" not prevents finding 
"drawback_nonexist.noassoc foo.shlexec" on wine */
+testfile = "%s\\drawback_nonexist.noassoc foo.shlexec";
+expected = 33; sprintf(fileA, testfile, tmpdir);
+rc=shell_execute(NULL, fileA, NULL, NULL);
+TEST_LPFILE_PARSED_OK();
+
+/* is SEE_MASK_DOENVSUBST default flag? Should only be when XP emulates 9x 
(XP bug or real 95 or ME behavior ?) */
+testfile = "%%TEMP%%\\%s\\simple.shlexec";
+expected = 2; sprintf(fileA, t